add 'clear' and 'byteslice' as an unsafe string methods #12054

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
@rajcybage
Contributor

rajcybage commented Aug 28, 2013

clear and byteslice are also unsafe string methods

@rajcybage

This comment has been minimized.

Show comment
Hide comment
@@ -84,7 +84,7 @@ module ActiveSupport #:nodoc:
class SafeBuffer < String
UNSAFE_STRING_METHODS = %w(
capitalize chomp chop delete downcase gsub lstrip next reverse rstrip
- slice squeeze strip sub succ swapcase tr tr_s upcase prepend
+ slice squeeze strip sub succ swapcase tr tr_s upcase prepend clear byteslice

This comment has been minimized.

@gardenofwine

gardenofwine Aug 30, 2013

Could you elaborate on why clear and byteslice should change the html_safe? status of a string? specifically, can you provide examples of an html_safe string that can become non safe because of the usage of these two methods?

@gardenofwine

gardenofwine Aug 30, 2013

Could you elaborate on why clear and byteslice should change the html_safe? status of a string? specifically, can you provide examples of an html_safe string that can become non safe because of the usage of these two methods?

This comment has been minimized.

@rajcybage

rajcybage Aug 30, 2013

Contributor

irb(main):046:0> s="asdasd&&&<>".html_safe
=> "asdasd&&&<>"
irb(main):047:0> s.html_safe?
=> true
irb(main):048:0> s.byteslice(8).html_safe?
=> nil
irb(main):053:0> s.byteslice(8..10)
=> "&<>"
irb(main):054:0> s.byteslice(8..10).html_safe?
=> nil

@rajcybage

rajcybage Aug 30, 2013

Contributor

irb(main):046:0> s="asdasd&&&<>".html_safe
=> "asdasd&&&<>"
irb(main):047:0> s.html_safe?
=> true
irb(main):048:0> s.byteslice(8).html_safe?
=> nil
irb(main):053:0> s.byteslice(8..10)
=> "&<>"
irb(main):054:0> s.byteslice(8..10).html_safe?
=> nil

This comment has been minimized.

@rajcybage

rajcybage Aug 30, 2013

Contributor

irb(main):060:0> s="asasasa".clear.html_safe?
=> false

@rajcybage

rajcybage Aug 30, 2013

Contributor

irb(main):060:0> s="asasasa".clear.html_safe?
=> false

@carlosantoniodasilva

This comment has been minimized.

Show comment
Hide comment
@carlosantoniodasilva

carlosantoniodasilva Dec 3, 2013

Member

/cc @fxn can you take a look please?

/cc @fxn can you take a look please?

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Feb 28, 2014

Contributor

Did this ever get a second look? /cc @fxn @carlosantoniodasilva

Contributor

JonRowe commented Feb 28, 2014

Did this ever get a second look? /cc @fxn @carlosantoniodasilva

@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn Feb 28, 2014

Member

I guess before we approve this or not, we need regression tests to illustrate the behaviour that should not be allowed anymore.

Member

arthurnn commented Feb 28, 2014

I guess before we approve this or not, we need regression tests to illustrate the behaviour that should not be allowed anymore.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Feb 6, 2015

Member

For what I got byteslice method already return an unsafe string when called. So there is no reason to add it to this array.

clear doesn't not change the status but since it remove all characters there is no reason to make it unsafe.

Thank you for the pull request.

Member

rafaelfranca commented Feb 6, 2015

For what I got byteslice method already return an unsafe string when called. So there is no reason to add it to this array.

clear doesn't not change the status but since it remove all characters there is no reason to make it unsafe.

Thank you for the pull request.

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