rack-devel archive mirror (unofficial) https://groups.google.com/group/rack-devel
 help / color / mirror / code / Atom feed
* Questions about the Prohibition of String Subclasses in responses
@ 2009-10-05 21:58 Michael Koziarski
  2009-10-05 23:02 ` Eric Wong
  0 siblings, 1 reply; 8+ messages in thread
From: Michael Koziarski @ 2009-10-05 21:58 UTC (permalink / raw)
  To: Rack Development


Hey Guys,

As I prepare to merge the new on-by-default XSS protection into rails
I'm bumping up against a constraint in rack, and I'm having trouble
figuring out why it's there.

http://github.com/chneukirchen/rack/blob/master/lib/rack/lint.rb#L465-475

Our XSS safe responses will fail this test because they return an
instance of ActionView::SafeBuffer which is a subclass of String.  We
use a subclass so that we can make all the concat and append
operations escape their arguments.

What's the rationale for preventing me from sending a subclass of
string?  Can we just change this stuff to use Object#kind_of? instead?

Cheers

Koz

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Questions about the Prohibition of String Subclasses in responses
  2009-10-05 21:58 Questions about the Prohibition of String Subclasses in responses Michael Koziarski
@ 2009-10-05 23:02 ` Eric Wong
  2009-10-05 23:22   ` Eric Wong
  0 siblings, 1 reply; 8+ messages in thread
From: Eric Wong @ 2009-10-05 23:02 UTC (permalink / raw)
  To: rack-devel


Michael Koziarski <koziarski@gmail.com> wrote:
> 
> Hey Guys,
> 
> As I prepare to merge the new on-by-default XSS protection into rails
> I'm bumping up against a constraint in rack, and I'm having trouble
> figuring out why it's there.
> 
> http://github.com/chneukirchen/rack/blob/master/lib/rack/lint.rb#L465-475
> 
> Our XSS safe responses will fail this test because they return an
> instance of ActionView::SafeBuffer which is a subclass of String.  We
> use a subclass so that we can make all the concat and append
> operations escape their arguments.
> 
> What's the rationale for preventing me from sending a subclass of
> string?

Not speaking for anyone else here, but this may break C extensions at
this point.  It was probably done this way to make extensions easier to
implement.  The core Ruby IO functions all call rb_obj_as_string() to
convert their arguments to strings, but some extensions out there may
not[1].


[1] *my* C extensions are safe against this, of course :)

-- 
Eric Wong

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Questions about the Prohibition of String Subclasses in responses
  2009-10-05 23:02 ` Eric Wong
@ 2009-10-05 23:22   ` Eric Wong
  2009-10-06  0:38     ` Michael Koziarski
  2009-10-06  8:05     ` James Tucker
  0 siblings, 2 replies; 8+ messages in thread
From: Eric Wong @ 2009-10-05 23:22 UTC (permalink / raw)
  To: rack-devel


Eric Wong <normalperson@yhbt.net> wrote:
> Michael Koziarski <koziarski@gmail.com> wrote:
> > Our XSS safe responses will fail this test because they return an
> > instance of ActionView::SafeBuffer which is a subclass of String.  We
> > use a subclass so that we can make all the concat and append
> > operations escape their arguments.

What's the overhead of metadef-ing #concat/#append each String response
on an individual basis vs subclassing?  That could be an option...

Also (not knowing much about Rails or view internals), would it be
possible to make the response an Array-like object and then make all
concats/appends work on the Array-like object instead of the String-like
object?

  class SafeBuffer < Array

    def <<(string)
      super(escape(string))
    end

    ...

  end

And then probably do a #join when responding for performance reasons:

  [ status, headers, [ safe_buffer.join('') ] ]

> > What's the rationale for preventing me from sending a subclass of
> > string?
> 
> Not speaking for anyone else here, but this may break C extensions at
> this point.  It was probably done this way to make extensions easier to
> implement.  The core Ruby IO functions all call rb_obj_as_string() to
> convert their arguments to strings, but some extensions out there may
> not[1].
> 
> [1] *my* C extensions are safe against this, of course :)

But I just read the patch attached to LH ticket #78 and that relaxes
the Hash type check, too.  I actually depend on that being a real
Hash in at least C one extension :x

I'll of course update that extension for the new spec if I need to :>

-- 
Eric Wong

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Questions about the Prohibition of String Subclasses in responses
  2009-10-05 23:22   ` Eric Wong
@ 2009-10-06  0:38     ` Michael Koziarski
  2009-10-06  2:52       ` Eric Wong
  2009-10-06  8:05     ` James Tucker
  1 sibling, 1 reply; 8+ messages in thread
From: Michael Koziarski @ 2009-10-06  0:38 UTC (permalink / raw)
  To: rack-devel


> What's the overhead of metadef-ing #concat/#append each String response
> on an individual basis vs subclassing?  That could be an option...

That's not really an option, we're not going to work around rack by
dynamically extending every buffer used in every rails app :).

> Also (not knowing much about Rails or view internals), would it be
> possible to make the response an Array-like object and then make all
> concats/appends work on the Array-like object instead of the String-like
> object?
>
>  class SafeBuffer < Array
>
>    def <<(string)
>      super(escape(string))
>    end
>
>    ...
>
>  end

We could do this, however ERB uses .to_s EVERYWHERE and it's very
tricky to get anything but a string out the other end.  I'd rather not
go down that path if we can avoid it at all.

I guess fundamentally I'm still not seeing a reason for the current
restriction to no-child-classes beyond 'might break C-extensions'.
What kind of code is in those c extensions which might break? We could
verify our subclasses with that same kind of code and see what
happens.  The whole purpose of inheritance is to ensure subtypes can
be used interchangably, we've put some effort into making sure we're
'liskov-approved'[1] and I'd be surprised if anything breaks.

Having said that, I've been surprised before

> But I just read the patch attached to LH ticket #78 and that relaxes
> the Hash type check, too.  I actually depend on that being a real
> Hash in at least C one extension :x

I'm not too worried on the Hash front, but it still seems wrong to
make the assumption that consumer classes can't handle subtypes

> I'll of course update that extension for the new spec if I need to :>
>
> --
> Eric Wong
>

[1] http://en.wikipedia.org/wiki/Liskov_substitution_principle


-- 
Cheers

Koz

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Questions about the Prohibition of String Subclasses in responses
  2009-10-06  0:38     ` Michael Koziarski
@ 2009-10-06  2:52       ` Eric Wong
  2009-10-06  6:40         ` Michael Koziarski
  0 siblings, 1 reply; 8+ messages in thread
From: Eric Wong @ 2009-10-06  2:52 UTC (permalink / raw)
  To: rack-devel


Michael Koziarski <michael@koziarski.com> wrote:
> I guess fundamentally I'm still not seeing a reason for the current
> restriction to no-child-classes beyond 'might break C-extensions'.
> What kind of code is in those c extensions which might break? We could
> verify our subclasses with that same kind of code and see what
> happens.  The whole purpose of inheritance is to ensure subtypes can
> be used interchangably, we've put some effort into making sure we're
> 'liskov-approved'[1] and I'd be surprised if anything breaks.

Actually, I think the vast majority of subclasses are safe.

Very contrived, but only(?) subclasses implemented in C that redefine
their internal representation to something incompatible with their
(expected) superclass would get broken (when accessed by other C
extensions).

Fortunately I don't know of any extensions that crazy as there's no
point in subclassing at that point.  And if there are, maybe people
using them deserve to get their apps broken :>

> Having said that, I've been surprised before

Part of me wants to apply this change and stick it into a release just
to see what (if anything it) breaks.  Fortunately for the rest of you
I'm not part of the Rack core team :)

-- 
Eric Wong

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Questions about the Prohibition of String Subclasses in responses
  2009-10-06  2:52       ` Eric Wong
@ 2009-10-06  6:40         ` Michael Koziarski
  2009-10-06 14:23           ` James Tucker
  0 siblings, 1 reply; 8+ messages in thread
From: Michael Koziarski @ 2009-10-06  6:40 UTC (permalink / raw)
  To: rack-devel


> Fortunately I don't know of any extensions that crazy as there's no
> point in subclassing at that point.  And if there are, maybe people
> using them deserve to get their apps broken :>

Seems very unlikely yeah.

>> Having said that, I've been surprised before
>
> Part of me wants to apply this change and stick it into a release just
> to see what (if anything it) breaks.  Fortunately for the rest of you
> I'm not part of the Rack core team :)

ditto on all points :)

-- 
Cheers

Koz

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Questions about the Prohibition of String Subclasses in responses
  2009-10-05 23:22   ` Eric Wong
  2009-10-06  0:38     ` Michael Koziarski
@ 2009-10-06  8:05     ` James Tucker
  1 sibling, 0 replies; 8+ messages in thread
From: James Tucker @ 2009-10-06  8:05 UTC (permalink / raw)
  To: rack-devel



On 6 Oct 2009, at 00:22, Eric Wong wrote:

>
> Eric Wong <normalperson@yhbt.net> wrote:
>> Michael Koziarski <koziarski@gmail.com> wrote:
>>> Our XSS safe responses will fail this test because they return an
>>> instance of ActionView::SafeBuffer which is a subclass of String.   
>>> We
>>> use a subclass so that we can make all the concat and append
>>> operations escape their arguments.
>
> What's the overhead of metadef-ing #concat/#append each String  
> response
> on an individual basis vs subclassing?  That could be an option...

Busting the method cache afaik.

>
> Also (not knowing much about Rails or view internals), would it be
> possible to make the response an Array-like object and then make all
> concats/appends work on the Array-like object instead of the String- 
> like
> object?
>
>  class SafeBuffer < Array
>
>    def <<(string)
>      super(escape(string))
>    end
>
>    ...
>
>  end
>
> And then probably do a #join when responding for performance reasons:
>
>  [ status, headers, [ safe_buffer.join('') ] ]
>
>>> What's the rationale for preventing me from sending a subclass of
>>> string?
>>
>> Not speaking for anyone else here, but this may break C extensions at
>> this point.  It was probably done this way to make extensions  
>> easier to
>> implement.  The core Ruby IO functions all call rb_obj_as_string() to
>> convert their arguments to strings, but some extensions out there may
>> not[1].
>>
>> [1] *my* C extensions are safe against this, of course :)
>
> But I just read the patch attached to LH ticket #78 and that relaxes
> the Hash type check, too.  I actually depend on that being a real
> Hash in at least C one extension :x
>
> I'll of course update that extension for the new spec if I need to :>
>
> -- 
> Eric Wong

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Questions about the Prohibition of String Subclasses in responses
  2009-10-06  6:40         ` Michael Koziarski
@ 2009-10-06 14:23           ` James Tucker
  0 siblings, 0 replies; 8+ messages in thread
From: James Tucker @ 2009-10-06 14:23 UTC (permalink / raw)
  To: rack-devel



On 6 Oct 2009, at 07:40, Michael Koziarski wrote:

>
>> Fortunately I don't know of any extensions that crazy as there's no
>> point in subclassing at that point.  And if there are, maybe people
>> using them deserve to get their apps broken :>
>
> Seems very unlikely yeah.
>
>>> Having said that, I've been surprised before
>>
>> Part of me wants to apply this change and stick it into a release  
>> just
>> to see what (if anything it) breaks.  Fortunately for the rest of you
>> I'm not part of the Rack core team :)
>
> ditto on all points :)

I've just moved us over to kind_of in Lint from instance_of, in all  
cases of instance_of. Generally speaking the spec needs to ensure that  
the types supplied can be sent straight to IO.

I think if anyone uses a type to breach this, they should find the  
bugs appear in their own apps for reasonably well expected reasons,  
although most errors will show up in the handlers themselves.

I've also cherry picked for the 1.0.1 release.

>
> -- 
> Cheers
>
> Koz

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2009-10-06 14:23 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-10-05 21:58 Questions about the Prohibition of String Subclasses in responses Michael Koziarski
2009-10-05 23:02 ` Eric Wong
2009-10-05 23:22   ` Eric Wong
2009-10-06  0:38     ` Michael Koziarski
2009-10-06  2:52       ` Eric Wong
2009-10-06  6:40         ` Michael Koziarski
2009-10-06 14:23           ` James Tucker
2009-10-06  8:05     ` James Tucker

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/rack.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).