[RFC/PATCH] lint: additional response checking/skipping for hijack

[RFC/PATCH] lint: additional response checking/skipping for hijack

[Eric Wong]

Not a serious patch for now, at least not all of it.
I suspect middlewares will break badly if the body.each/body.close
checks are enforced.

---
 lib/rack/lint.rb | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/lib/rack/lint.rb b/lib/rack/lint.rb
index 1bc2127..f895772 100644
--- a/lib/rack/lint.rb
+++ b/lib/rack/lint.rb
@@ -9,6 +9,7 @@ class Lint
     def initialize(app)
       @app = app
       @content_length = nil
+      @response_hijacked = false
     end
 
     # :stopdoc:
@@ -47,6 +48,15 @@ def _call(env)
 
       ## and returns an Array of exactly three values:
       status, headers, @body = @app.call(env)
+
+      # hijacked requests may not give a valid response, do not check them
+      if env.include?("rack.hijack_io")
+        # request hijacking implies response hijacking, this will ensure
+        # the response body raises if body.each or body.close gets called
+        @response_hijacked = true
+        return [ status, headers, self ]
+      end
+
       ## The *status*,
       check_status status
       ## the *headers*,
@@ -530,6 +540,7 @@ def check_hijack_response(headers, env)
         headers['rack.hijack'] = proc do |io|
           original_hijack.call HijackWrapper.new(io)
         end
+        @response_hijacked = true
       else
         ## 
         ## The special response header <tt>rack.hijack</tt> must only be set
@@ -636,6 +647,9 @@ def verify_content_length(bytes)
 
     ## === The Body
     def each
+      assert("server is not attempting to iterate hijacked response body") {
+        @response_hijacked == false
+      }
       @closed = false
       bytes = 0
 
@@ -683,6 +697,9 @@ def each
     end
 
     def close
+      assert("server is not attempting to close hijacked response") {
+        @response_hijacked == false
+      }
       @closed = true
       @body.close  if @body.respond_to?(:close)
     end
-- 
Eric Wong

Re: [RFC/PATCH] lint: additional response checking/skipping for hijack

[James Tucker]

[-- Attachment #1: Type: text/plain, Size: 2477 bytes --]

I'd generally recommend that the hijacking app return valid stub data,
something like: [200, {}, []].


On 22 January 2013 16:20, Eric Wong <normalperson@yhbt.net> wrote:

> Not a serious patch for now, at least not all of it.
> I suspect middlewares will break badly if the body.each/body.close
> checks are enforced.
>
> ---
>  lib/rack/lint.rb | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/lib/rack/lint.rb b/lib/rack/lint.rb
> index 1bc2127..f895772 100644
> --- a/lib/rack/lint.rb
> +++ b/lib/rack/lint.rb
> @@ -9,6 +9,7 @@ class Lint
>      def initialize(app)
>        @app = app
>        @content_length = nil
> +      @response_hijacked = false
>      end
>
>      # :stopdoc:
> @@ -47,6 +48,15 @@ def _call(env)
>
>        ## and returns an Array of exactly three values:
>        status, headers, @body = @app.call(env)
> +
> +      # hijacked requests may not give a valid response, do not check them
> +      if env.include?("rack.hijack_io")
> +        # request hijacking implies response hijacking, this will ensure
> +        # the response body raises if body.each or body.close gets called
> +        @response_hijacked = true
> +        return [ status, headers, self ]
> +      end
> +
>        ## The *status*,
>        check_status status
>        ## the *headers*,
> @@ -530,6 +540,7 @@ def check_hijack_response(headers, env)
>          headers['rack.hijack'] = proc do |io|
>            original_hijack.call HijackWrapper.new(io)
>          end
> +        @response_hijacked = true
>        else
>          ##
>          ## The special response header <tt>rack.hijack</tt> must only be
> set
> @@ -636,6 +647,9 @@ def verify_content_length(bytes)
>
>      ## === The Body
>      def each
> +      assert("server is not attempting to iterate hijacked response
> body") {
> +        @response_hijacked == false
> +      }
>        @closed = false
>        bytes = 0
>
> @@ -683,6 +697,9 @@ def each
>      end
>
>      def close
> +      assert("server is not attempting to close hijacked response") {
> +        @response_hijacked == false
> +      }
>        @closed = true
>        @body.close  if @body.respond_to?(:close)
>      end
> --
> Eric Wong
>

-- 

--- 
You received this message because you are subscribed to the Google Groups "Rack Development" group.
To unsubscribe from this group, send email to rack-devel+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.



[-- Attachment #2: Type: text/html, Size: 3239 bytes --]

Re: [RFC/PATCH] lint: additional response checking/skipping for hijack

[Tim Carey-Smith]

Could you update the examples on the original issue to behave in this way?
This is a point of confusion, I think. 

Perhaps this could be added to the SPEC as well?

On Jan 29, 2013, at 11:01 AM, James Tucker <jftucker@gmail.com> wrote:

> I'd generally recommend that the hijacking app return valid stub data,
> something like: [200, {}, []].
> 
> 
> On 22 January 2013 16:20, Eric Wong <normalperson@yhbt.net> wrote:
> 
>> Not a serious patch for now, at least not all of it.
>> I suspect middlewares will break badly if the body.each/body.close
>> checks are enforced.
>> 
>> ---
>> lib/rack/lint.rb | 17 +++++++++++++++++
>> 1 file changed, 17 insertions(+)
>> 
>> diff --git a/lib/rack/lint.rb b/lib/rack/lint.rb
>> index 1bc2127..f895772 100644
>> --- a/lib/rack/lint.rb
>> +++ b/lib/rack/lint.rb
>> @@ -9,6 +9,7 @@ class Lint
>>     def initialize(app)
>>       @app = app
>>       @content_length = nil
>> +      @response_hijacked = false
>>     end
>> 
>>     # :stopdoc:
>> @@ -47,6 +48,15 @@ def _call(env)
>> 
>>       ## and returns an Array of exactly three values:
>>       status, headers, @body = @app.call(env)
>> +
>> +      # hijacked requests may not give a valid response, do not check them
>> +      if env.include?("rack.hijack_io")
>> +        # request hijacking implies response hijacking, this will ensure
>> +        # the response body raises if body.each or body.close gets called
>> +        @response_hijacked = true
>> +        return [ status, headers, self ]
>> +      end
>> +
>>       ## The *status*,
>>       check_status status
>>       ## the *headers*,
>> @@ -530,6 +540,7 @@ def check_hijack_response(headers, env)
>>         headers['rack.hijack'] = proc do |io|
>>           original_hijack.call HijackWrapper.new(io)
>>         end
>> +        @response_hijacked = true
>>       else
>>         ##
>>         ## The special response header <tt>rack.hijack</tt> must only be
>> set
>> @@ -636,6 +647,9 @@ def verify_content_length(bytes)
>> 
>>     ## === The Body
>>     def each
>> +      assert("server is not attempting to iterate hijacked response
>> body") {
>> +        @response_hijacked == false
>> +      }
>>       @closed = false
>>       bytes = 0
>> 
>> @@ -683,6 +697,9 @@ def each
>>     end
>> 
>>     def close
>> +      assert("server is not attempting to close hijacked response") {
>> +        @response_hijacked == false
>> +      }
>>       @closed = true
>>       @body.close  if @body.respond_to?(:close)
>>     end
>> --
>> Eric Wong
>> 
> 
> -- 
> 
> --- 
> You received this message because you are subscribed to the Google Groups "Rack Development" group.
> To unsubscribe from this group, send email to rack-devel+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
> 
> 

-- 

--- 
You received this message because you are subscribed to the Google Groups "Rack Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rack-devel+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [RFC/PATCH] lint: additional response checking/skipping for hijack

[James Tucker]

[-- Attachment #1: Type: text/plain, Size: 3721 bytes --]

The examples do work this way.

What are you referring to?


On Sun, Apr 21, 2013 at 8:41 AM, Tim Carey-Smith <g@spork.in> wrote:

> Could you update the examples on the original issue to behave in this way?
> This is a point of confusion, I think.
>
> Perhaps this could be added to the SPEC as well?
>
> On Jan 29, 2013, at 11:01 AM, James Tucker <jftucker@gmail.com> wrote:
>
> > I'd generally recommend that the hijacking app return valid stub data,
> > something like: [200, {}, []].
> >
> >
> > On 22 January 2013 16:20, Eric Wong <normalperson@yhbt.net> wrote:
> >
> >> Not a serious patch for now, at least not all of it.
> >> I suspect middlewares will break badly if the body.each/body.close
> >> checks are enforced.
> >>
> >> ---
> >> lib/rack/lint.rb | 17 +++++++++++++++++
> >> 1 file changed, 17 insertions(+)
> >>
> >> diff --git a/lib/rack/lint.rb b/lib/rack/lint.rb
> >> index 1bc2127..f895772 100644
> >> --- a/lib/rack/lint.rb
> >> +++ b/lib/rack/lint.rb
> >> @@ -9,6 +9,7 @@ class Lint
> >>     def initialize(app)
> >>       @app = app
> >>       @content_length = nil
> >> +      @response_hijacked = false
> >>     end
> >>
> >>     # :stopdoc:
> >> @@ -47,6 +48,15 @@ def _call(env)
> >>
> >>       ## and returns an Array of exactly three values:
> >>       status, headers, @body = @app.call(env)
> >> +
> >> +      # hijacked requests may not give a valid response, do not check
> them
> >> +      if env.include?("rack.hijack_io")
> >> +        # request hijacking implies response hijacking, this will
> ensure
> >> +        # the response body raises if body.each or body.close gets
> called
> >> +        @response_hijacked = true
> >> +        return [ status, headers, self ]
> >> +      end
> >> +
> >>       ## The *status*,
> >>       check_status status
> >>       ## the *headers*,
> >> @@ -530,6 +540,7 @@ def check_hijack_response(headers, env)
> >>         headers['rack.hijack'] = proc do |io|
> >>           original_hijack.call HijackWrapper.new(io)
> >>         end
> >> +        @response_hijacked = true
> >>       else
> >>         ##
> >>         ## The special response header <tt>rack.hijack</tt> must only be
> >> set
> >> @@ -636,6 +647,9 @@ def verify_content_length(bytes)
> >>
> >>     ## === The Body
> >>     def each
> >> +      assert("server is not attempting to iterate hijacked response
> >> body") {
> >> +        @response_hijacked == false
> >> +      }
> >>       @closed = false
> >>       bytes = 0
> >>
> >> @@ -683,6 +697,9 @@ def each
> >>     end
> >>
> >>     def close
> >> +      assert("server is not attempting to close hijacked response") {
> >> +        @response_hijacked == false
> >> +      }
> >>       @closed = true
> >>       @body.close  if @body.respond_to?(:close)
> >>     end
> >> --
> >> Eric Wong
> >>
> >
> > --
> >
> > ---
> > You received this message because you are subscribed to the Google
> Groups "Rack Development" group.
> > To unsubscribe from this group, send email to
> rack-devel+unsubscribe@googlegroups.com.
> > For more options, visit https://groups.google.com/groups/opt_out.
> >
> >
>
> --
>
> ---
> You received this message because you are subscribed to the Google Groups
> "Rack Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to rack-devel+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>
>

-- 

--- 
You received this message because you are subscribed to the Google Groups "Rack Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rack-devel+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.



[-- Attachment #2: Type: text/html, Size: 5484 bytes --]

Re: [RFC/PATCH] lint: additional response checking/skipping for hijack

[Tim Carey-Smith]

The referenced hijack.ru app returns the IO object instead of returning [200, {}, []]. 

https://github.com/raggi/thin/blob/e04855459cb42fd98a0a483075f8337cafe6d949/example/hijack.ru#L15

Thanks,
Tim

On Apr 23, 2013, at 1:37 PM, James Tucker <jftucker@gmail.com> wrote:

> The examples do work this way.
> 
> What are you referring to?
> 
> 
> On Sun, Apr 21, 2013 at 8:41 AM, Tim Carey-Smith <g@spork.in> wrote:
> 
>> Could you update the examples on the original issue to behave in this way?
>> This is a point of confusion, I think.
>> 
>> Perhaps this could be added to the SPEC as well?
>> 
>> On Jan 29, 2013, at 11:01 AM, James Tucker <jftucker@gmail.com> wrote:
>> 
>>> I'd generally recommend that the hijacking app return valid stub data,
>>> something like: [200, {}, []].
>>> 
>>> 
>>> On 22 January 2013 16:20, Eric Wong <normalperson@yhbt.net> wrote:
>>> 
>>>> Not a serious patch for now, at least not all of it.
>>>> I suspect middlewares will break badly if the body.each/body.close
>>>> checks are enforced.
>>>> 
>>>> ---
>>>> lib/rack/lint.rb | 17 +++++++++++++++++
>>>> 1 file changed, 17 insertions(+)
>>>> 
>>>> diff --git a/lib/rack/lint.rb b/lib/rack/lint.rb
>>>> index 1bc2127..f895772 100644
>>>> --- a/lib/rack/lint.rb
>>>> +++ b/lib/rack/lint.rb
>>>> @@ -9,6 +9,7 @@ class Lint
>>>>    def initialize(app)
>>>>      @app = app
>>>>      @content_length = nil
>>>> +      @response_hijacked = false
>>>>    end
>>>> 
>>>>    # :stopdoc:
>>>> @@ -47,6 +48,15 @@ def _call(env)
>>>> 
>>>>      ## and returns an Array of exactly three values:
>>>>      status, headers, @body = @app.call(env)
>>>> +
>>>> +      # hijacked requests may not give a valid response, do not check
>> them
>>>> +      if env.include?("rack.hijack_io")
>>>> +        # request hijacking implies response hijacking, this will
>> ensure
>>>> +        # the response body raises if body.each or body.close gets
>> called
>>>> +        @response_hijacked = true
>>>> +        return [ status, headers, self ]
>>>> +      end
>>>> +
>>>>      ## The *status*,
>>>>      check_status status
>>>>      ## the *headers*,
>>>> @@ -530,6 +540,7 @@ def check_hijack_response(headers, env)
>>>>        headers['rack.hijack'] = proc do |io|
>>>>          original_hijack.call HijackWrapper.new(io)
>>>>        end
>>>> +        @response_hijacked = true
>>>>      else
>>>>        ##
>>>>        ## The special response header <tt>rack.hijack</tt> must only be
>>>> set
>>>> @@ -636,6 +647,9 @@ def verify_content_length(bytes)
>>>> 
>>>>    ## === The Body
>>>>    def each
>>>> +      assert("server is not attempting to iterate hijacked response
>>>> body") {
>>>> +        @response_hijacked == false
>>>> +      }
>>>>      @closed = false
>>>>      bytes = 0
>>>> 
>>>> @@ -683,6 +697,9 @@ def each
>>>>    end
>>>> 
>>>>    def close
>>>> +      assert("server is not attempting to close hijacked response") {
>>>> +        @response_hijacked == false
>>>> +      }
>>>>      @closed = true
>>>>      @body.close  if @body.respond_to?(:close)
>>>>    end
>>>> --
>>>> Eric Wong
>>>> 
>>> 
>>> --
>>> 
>>> ---
>>> You received this message because you are subscribed to the Google
>> Groups "Rack Development" group.
>>> To unsubscribe from this group, send email to
>> rack-devel+unsubscribe@googlegroups.com.
>>> For more options, visit https://groups.google.com/groups/opt_out.
>>> 
>>> 
>> 
>> --
>> 
>> ---
>> You received this message because you are subscribed to the Google Groups
>> "Rack Development" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to rack-devel+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/groups/opt_out.
>> 
>> 
>> 
> 
> -- 
> 
> --- 
> You received this message because you are subscribed to the Google Groups "Rack Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to rack-devel+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
> 
> 

-- 

--- 
You received this message because you are subscribed to the Google Groups "Rack Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rack-devel+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [RFC/PATCH] lint: additional response checking/skipping for hijack

[James Tucker]

[-- Attachment #1: Type: text/plain, Size: 5069 bytes --]

Hah, so it does. The other doesn't. That's what I get for writing teeny
examples.


On Mon, Apr 22, 2013 at 6:48 PM, Tim Carey-Smith <g@spork.in> wrote:

> The referenced hijack.ru app returns the IO object instead of returning
> [200, {}, []].
>
>
> https://github.com/raggi/thin/blob/e04855459cb42fd98a0a483075f8337cafe6d949/example/hijack.ru#L15
>
> Thanks,
> Tim
>
> On Apr 23, 2013, at 1:37 PM, James Tucker <jftucker@gmail.com> wrote:
>
> > The examples do work this way.
> >
> > What are you referring to?
> >
> >
> > On Sun, Apr 21, 2013 at 8:41 AM, Tim Carey-Smith <g@spork.in> wrote:
> >
> >> Could you update the examples on the original issue to behave in this
> way?
> >> This is a point of confusion, I think.
> >>
> >> Perhaps this could be added to the SPEC as well?
> >>
> >> On Jan 29, 2013, at 11:01 AM, James Tucker <jftucker@gmail.com> wrote:
> >>
> >>> I'd generally recommend that the hijacking app return valid stub data,
> >>> something like: [200, {}, []].
> >>>
> >>>
> >>> On 22 January 2013 16:20, Eric Wong <normalperson@yhbt.net> wrote:
> >>>
> >>>> Not a serious patch for now, at least not all of it.
> >>>> I suspect middlewares will break badly if the body.each/body.close
> >>>> checks are enforced.
> >>>>
> >>>> ---
> >>>> lib/rack/lint.rb | 17 +++++++++++++++++
> >>>> 1 file changed, 17 insertions(+)
> >>>>
> >>>> diff --git a/lib/rack/lint.rb b/lib/rack/lint.rb
> >>>> index 1bc2127..f895772 100644
> >>>> --- a/lib/rack/lint.rb
> >>>> +++ b/lib/rack/lint.rb
> >>>> @@ -9,6 +9,7 @@ class Lint
> >>>>    def initialize(app)
> >>>>      @app = app
> >>>>      @content_length = nil
> >>>> +      @response_hijacked = false
> >>>>    end
> >>>>
> >>>>    # :stopdoc:
> >>>> @@ -47,6 +48,15 @@ def _call(env)
> >>>>
> >>>>      ## and returns an Array of exactly three values:
> >>>>      status, headers, @body = @app.call(env)
> >>>> +
> >>>> +      # hijacked requests may not give a valid response, do not check
> >> them
> >>>> +      if env.include?("rack.hijack_io")
> >>>> +        # request hijacking implies response hijacking, this will
> >> ensure
> >>>> +        # the response body raises if body.each or body.close gets
> >> called
> >>>> +        @response_hijacked = true
> >>>> +        return [ status, headers, self ]
> >>>> +      end
> >>>> +
> >>>>      ## The *status*,
> >>>>      check_status status
> >>>>      ## the *headers*,
> >>>> @@ -530,6 +540,7 @@ def check_hijack_response(headers, env)
> >>>>        headers['rack.hijack'] = proc do |io|
> >>>>          original_hijack.call HijackWrapper.new(io)
> >>>>        end
> >>>> +        @response_hijacked = true
> >>>>      else
> >>>>        ##
> >>>>        ## The special response header <tt>rack.hijack</tt> must only
> be
> >>>> set
> >>>> @@ -636,6 +647,9 @@ def verify_content_length(bytes)
> >>>>
> >>>>    ## === The Body
> >>>>    def each
> >>>> +      assert("server is not attempting to iterate hijacked response
> >>>> body") {
> >>>> +        @response_hijacked == false
> >>>> +      }
> >>>>      @closed = false
> >>>>      bytes = 0
> >>>>
> >>>> @@ -683,6 +697,9 @@ def each
> >>>>    end
> >>>>
> >>>>    def close
> >>>> +      assert("server is not attempting to close hijacked response") {
> >>>> +        @response_hijacked == false
> >>>> +      }
> >>>>      @closed = true
> >>>>      @body.close  if @body.respond_to?(:close)
> >>>>    end
> >>>> --
> >>>> Eric Wong
> >>>>
> >>>
> >>> --
> >>>
> >>> ---
> >>> You received this message because you are subscribed to the Google
> >> Groups "Rack Development" group.
> >>> To unsubscribe from this group, send email to
> >> rack-devel+unsubscribe@googlegroups.com.
> >>> For more options, visit https://groups.google.com/groups/opt_out.
> >>>
> >>>
> >>
> >> --
> >>
> >> ---
> >> You received this message because you are subscribed to the Google
> Groups
> >> "Rack Development" group.
> >> To unsubscribe from this group and stop receiving emails from it, send
> an
> >> email to rack-devel+unsubscribe@googlegroups.com.
> >> For more options, visit https://groups.google.com/groups/opt_out.
> >>
> >>
> >>
> >
> > --
> >
> > ---
> > You received this message because you are subscribed to the Google
> Groups "Rack Development" group.
> > To unsubscribe from this group and stop receiving emails from it, send
> an email to rack-devel+unsubscribe@googlegroups.com.
> > For more options, visit https://groups.google.com/groups/opt_out.
> >
> >
>
> --
>
> ---
> You received this message because you are subscribed to the Google Groups
> "Rack Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to rack-devel+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>
>

-- 

--- 
You received this message because you are subscribed to the Google Groups "Rack Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rack-devel+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.



[-- Attachment #2: Type: text/html, Size: 8121 bytes --]