Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix for SafeBuffer#gsub and magic match globs #1622

Closed
wants to merge 4 commits into from

6 participants

@tardate

(NB: so far not a viable fix .. still looking for solutions).
Was only going to be partial anyway:

  • $1, $2, $`, $&, and $’ now available within a block passed to SafeBuffer#gsub
  • documented limitation that they are not available after the SafeBuffer#gsub call See GH#1555 for the original issue report
@tardate tardate Fix for SafeBuffer#gsub and magic match globs
Only partial I'm afraid:
* $1, $2, $`, $&, and $’ now available within a block passed to gsub
* documented limitation that they are not available after the gsub call
* see GH#1555
5107f39
...t/lib/active_support/core_ext/string/output_safety.rb
@@ -116,6 +116,22 @@ module ActiveSupport #:nodoc:
end
EOT
end
+
+ # Provides a special-case override for String#gsub
+ # to preserve some of the standard behaviour with respect
+ # to magic matching global variables
+ #
+ # Unlike standard gsub, magic matching globs are _not_
+ # available in code following a SafeBuffer#gsub call,
+ # but they are available within a block passed to gsub, eg:
+ # SafeBuffer.new('m').gsub(/(m)/){ $1 }
+ # In this case $1 == 'm' within the block, but not afterwards
+ #
+ # If you really need the magic matching variables after the gsub call
+ # you will need to covert SafeBuffer to a String first
+ def gsub(*args)
+ String.new(self).gsub(*args)
@dmathieu Collaborator

=> self.dup.gsub(*args)

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

I don't think that's the appropriate approach.

@tardate

@dmathiew self.dup gets you back a SafeBuffer and thus into infinite recursion. I was looking for a better way than String.new for getting the unsafe string from a SafeBuffer but I had a blank. Can't to_s because that just gives you back self also..
doh,
to_str.gsub(*args)
is what I wanted

@tardate tardate Fix for SafeBuffer#gsub and magic match globs
* to_str in preference of String.new
* tests + metaphorical exactitude
1fa1134
...t/lib/active_support/core_ext/string/output_safety.rb
@@ -116,6 +116,22 @@ module ActiveSupport #:nodoc:
end
EOT
end
+
+ # Provides a special-case override for String#gsub
+ # to preserve some of the standard behaviour with respect
+ # to magic matching global variables
+ #
+ # Unlike standard gsub, magic matching globs are _not_
+ # available in code following a SafeBuffer#gsub call,
+ # but they are available within a block passed to gsub, eg:
+ # SafeBuffer.new('m').gsub(/(m)/){ $1 }
+ # In this case $1 == 'm' within the block, but not afterwards
+ #
+ # If you really need the magic matching variables after the gsub call
+ # you will need to covert SafeBuffer to a String first
@vijaydev Collaborator

convert :-)

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

I'm not excited about this solution because it changes the return value. Before this patch, the return type from gsub would be a SafeBuffer instance, now it will just be a String.

@nzkoz what's your opinion?

@NZKoz
Owner

I don't think we should bother trying to support this stuff. While SafeBuffer is a String subclass, that's mostly for erubis and rack compatibility reasons. In hindsight those reasons aren't really good enough and we could, and probably should, switch to a buffer that doesn't descend from string.

Basically the buffers aren't meant to be used for all these kinds of operations and attempting to use them should probably be considered a bug in the app. That's my 'grumpy old man' position on it, but at the same time if there's a nice and elegant fix I'm all for it.

@tardate

NB: this patch doesn't actually introduce the constraint "Should not return safe buffer from gsub"; actually that new constraint is what instigated this as a "root cause" fix rather than having to go around and fix all the symptoms. I wasn't following the original change but I guess the reasoning was that after a gsub it is not predictable whether the result is safe any more.

This patch just provides special-case handling for gsub to ensure magic globs are preserved rather than fall back on the general UNSAFE_STRING_METHODS.each {..} dynamic wrapper generation routine (which loses magic globs). Use of $1 in a gsub block is pretty common and was responsible for the new SafeBuffer constraints breaking escape_javascript, Rack::Utils.escape and so on..

@NZKoz
Owner

yeah, the constraint of returning a String not a SafeBuffer is deliberate

@Roman2K

I think you forgot to forward the potential block here. Your test doesn't catch this since your assertion is placed within the block that isn't forwarded and thus never called.

@tardate

@Roman2k thank you! as my attempts to fix this morphed I got mislead by the test. But now I discover that passing the block just gets me back to square one - $1 not available.

So this is not a fix .. yet. I might give it one more shot before giving up. I'll close this pull request till then

@tardate tardate SafeBuffer#gsub test still failing for $1
* ensured the block is passed, refactor test so can't be mislead if block never called
62e2818
@tardate tardate closed this
@jake3030 jake3030 referenced this pull request from a commit in jake3030/rails
@mrj mrj Fixed incorrect parsing of query parameters with mixed-depth nesting …
…inside an array [#1622 state:resolved]

Signed-off-by: Frederick Cheung <frederick.cheung@gmail.com>
5138f75
@jake3030 jake3030 referenced this pull request from a commit in jake3030/rails
@mrj mrj Fixed incorrect parsing of query parameters with mixed-depth nesting …
…inside an array [#1622 state:resolved]

Signed-off-by: Frederick Cheung <frederick.cheung@gmail.com>
ea20901
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 10, 2011
  1. @tardate

    Fix for SafeBuffer#gsub and magic match globs

    tardate authored
    Only partial I'm afraid:
    * $1, $2, $`, $&, and $’ now available within a block passed to gsub
    * documented limitation that they are not available after the gsub call
    * see GH#1555
  2. @tardate

    Fix for SafeBuffer#gsub and magic match globs

    tardate authored
    * to_str in preference of String.new
    * tests + metaphorical exactitude
  3. @tardate
Commits on Jun 14, 2011
  1. @tardate

    SafeBuffer#gsub test still failing for $1

    tardate authored
    * ensured the block is passed, refactor test so can't be mislead if block never called
This page is out of date. Refresh to see the latest.
View
16 activesupport/lib/active_support/core_ext/string/output_safety.rb
@@ -116,6 +116,22 @@ def #{unsafe_method}!(*args)
end
EOT
end
+
+ # Provides a special-case override for String#gsub
+ # to preserve some of the standard behaviour with respect
+ # to magic matching global variables
+ #
+ # Unlike standard gsub, magic matching globs are _not_
+ # available in code following a SafeBuffer#gsub call,
+ # but they are available within a block passed to gsub, eg:
+ # SafeBuffer.new('m').gsub(/(m)/){ $1 }
+ # In this case $1 == 'm' within the block, but not afterwards
+ #
+ # If you really need the magic matching variables after the gsub call
+ # you will need to convert SafeBuffer to a String first
+ def gsub(*args, &block)
+ to_str.gsub(*args, &block)
+ end
end
end
View
20 activesupport/test/safe_buffer_test.rb
@@ -50,4 +50,24 @@ def setup
@buffer.gsub!('', 'asdf')
end
end
+
+ test "Should set magic match variables within block passed to gsub" do
+ 'burn'[/(matches)/]
+ @buffer << 'swan'
+ result = nil
+ @buffer.gsub(/(swan)/) { result = $1 }
+ assert_equal 'swan', result, "dang it, still not working"
+ end
+
+ test "Should not expect magic match variables after gsub call" do
+ 'burn'[/(matches)/]
+ @buffer << 'vesta'
+ @buffer.gsub(/(vesta)/, '')
+ assert !$1, %(
+ if you can make this test fail it is a _good_ thing: somehow you have
+ restored the standard behaviour of SafeBuffer#gsub to make magic matching
+ variables available after the call, and you could invert this test
+ )
+ end
+
end
Something went wrong with that request. Please try again.