rails: SafeBuffer#gsub breaks block form match variables $1, $2, $`, $&, and $’

commit 53a2c0baf2b128dd4808eca313256f6f4bb8c4cd introduced a change to ensure gsub returns an unsafe string.

When gsub is used in block form, variables such as $1, $2, $`, $&, and $’ will be set appropriately. Now it seems that SafeBuffer’s intervention loses the context and these variables are no longer available in the block.

This is the simplest test I can make to demonstrate the issue (which currently fails on 3.0-stable)…

class SafeBufferTest < ActiveSupport::TestCase
  def setup
    @buffer = ActiveSupport::SafeBuffer.new
  end
  [..]
  test "Should not break gsub block form match variables" do
    @buffer << 'matchme'
    @buffer.gsub(/(matchme)/) { assert_equal 'matchme', $1 }
  end

So far it’s know that this breaks escape_javascript since it uses $1 matcher (see https://github.com/rails/rails/issues/1553 ). I presume there may be others also affected.

Investigating what a good fix would be… seems awful heavy handed to have to capture and reset all those variables, but on the other hand, its bad form to leave a ruby core method different from it’s documented behaviour.

About this issue

  • Original URL
  • State: closed
  • Created 13 years ago
  • Reactions: 1
  • Comments: 34 (24 by maintainers)

Commits related to this issue

Most upvoted comments

@josevalim @tenderlove this issue should not have been closed, as it still exists in rails 5.1.6. I don’t know how many hours have been lost by people chasing down the issue over the years. Please reinstate the warnings that were inexplicably and irresponsibly reverted by https://github.com/rails/rails/commit/e05d4cea69919ed0a2e5832bde120b5d0f12c0ec#diff-f639a79f308e72f54af369291a4d5907

+6
gsar on Jun 13, 2018

I just ran into this issue again today (using rails 5.1.4) and wasted quite some time debugging before I realized ActiveSupport::SafeBuffer was the cause. It looks like I am not the only one who continues to get bitten by this.

It looks like this issue was closed by https://github.com/rails/rails/pull/2248 but then that fix was reverted (why?). I would suggest reopening this until a solution is finally merged. (Closing the issue suggests that it was resolved. If it was resolved, what was the resolution??)

Perhaps raise an error when gsub is used on a SafeBuffer, or at least emit a warning?

+4
TylerRick on Feb 13, 2018

Just ran into this issue again today (using Rails 5.0 and Ruby 2.4.1) and took quite some time before I realized ActiveSupport::SafeBuffer was the cause. I see there were several attempts at fixes, which were uncompleted or reverted. It would however be nice if we could at least show a warning when this is happening to prevent wasting time.

Note: this is also a problem when using Regexp.last_match (as did I).

+3
kreintjes on Aug 24, 2017

Just ran into this problem myself, and lucked into finding this thread. Wanted to add one important note for other travelers.

The solution provided by @dmathieu…

my_text.to_str.gsub {}

worked for me, however note that you must use to_str rather than to_s.

SafeBuffer inherits from String and overrides to_sto return self, but does not touch to_str, which is why it works in this case.

+3
BattleBrisket on Oct 8, 2014

I also just spent a good deal of my day tracking down this bug.

Issue #1555 was resolved by pull request #2248 which raises an exception for these methods, but this change was reverted on the same day for some reason. I think raising an exception when called with a block would be far more sensible than knowingly letting it run in a broken state?

+1
toupeira on Aug 19, 2013
Read more comments on GitHub
← rails: Running multiple migrations simultaneously can cause race conditions (on Postgres, probably on others)
rails: save (ActiveRecord::Persistence) can return true even when the model is not saved, and seems to break the principle of "least surprise" →