Merge pull request #479 from chadlwilson/1.2-backport-fixes

1.2: backport minor Rack 2.2-era spec compatibility fixes

Author: chadlwilson

CHANGELOG.md

@@ -2,6 +2,10 @@
 
 - fix: ensure `rack.` internal headers are stripped in responses
 - chore: remove ancient dead Rails 2-era adapter code
+- fix: ensure ErrorApp does not mutate shared headers constant
+- fix: JRuby::Rack::Input#read(0) should return an empty string
+- fix: close the original body when ShowStatus replaces it
+- fix: detect Transfer-Encoding/Content-Length headers case-insensitively
 
 ## 1.2.8
 

src/main/java/org/jruby/rack/ext/Input.java

@@ -126,8 +126,7 @@ else if ( arg instanceof RackEnvironment ) {
     @JRubyMethod()
     public IRubyObject gets(final ThreadContext context) {
         try {
-            final int NEWLINE = 10;
-            final byte[] bytes = readUntil(NEWLINE, 0);
+            final byte[] bytes = readUntil('\n', 0);
             if ( bytes != null ) {
                 return context.runtime.newString(new ByteList(bytes, false));
             }
@@ -155,14 +154,19 @@ public IRubyObject gets(final ThreadContext context) {
      */
     @JRubyMethod(optional = 2)
     public IRubyObject read(final ThreadContext context, final IRubyObject[] args) {
-        int readLen = 0;
-        if ( args.length > 0 ) {
+        int readLen = 0; boolean readAll = true;
+        if ( args.length > 0 && ! args[0].isNil() ) {
             long len = args[0].convertToInteger("to_i").getLongValue();
             readLen = (int) Math.min(len, Integer.MAX_VALUE);
+            readAll = false;
         }
         final RubyString buffer = args.length > 1 ? args[1].asString() : null;
+        if ( ! readAll && readLen <= 0 ) { // like IO#read - read(0) returns "" (not all data)
+            if ( buffer != null ) { buffer.clear(); return buffer; }
+            return RubyString.newEmptyString(context.runtime);
+        }
         try {
-            final byte[] bytes = readUntil(MATCH_NONE, readLen);
+            final byte[] bytes = readUntil(MATCH_NONE, readAll ? 0 : readLen);
             if ( bytes != null ) {
                 if ( buffer != null ) {
                     buffer.clear();
@@ -176,7 +180,7 @@ public IRubyObject read(final ThreadContext context, final IRubyObject[] args) {
                 }
                 return context.runtime.newString(new ByteList(bytes, false));
             }
-            return readLen > 0 ? context.nil : RubyString.newEmptyString(context.runtime);
+            return readAll ? RubyString.newEmptyString(context.runtime) : context.nil;
         }
         catch (IOException e) {
             throw ExceptionUtils.newIOError(context.runtime, e);

src/main/java/org/jruby/rack/ext/Logger.java

@@ -448,18 +448,12 @@ public IRubyObject format_severity(final ThreadContext context, final IRubyObjec
         return RubyString.newStringShared(context.runtime, formatSeverity(severity));
     }
 
-    private static final ByteList FORMATTED_DEBUG =
-            new ByteList(new byte[] { 'D','E','B','U','G' }, false);
-    private static final ByteList FORMATTED_INFO =
-            new ByteList(new byte[] { 'I','N','F','O' }, false);
-    private static final ByteList FORMATTED_WARN =
-            new ByteList(new byte[] { 'W','A','R','N' }, false);
-    private static final ByteList FORMATTED_ERROR =
-            new ByteList(new byte[] { 'E','R','R','O','R' }, false);
-    private static final ByteList FORMATTED_FATAL =
-            new ByteList(new byte[] { 'F','A','T','A','L' }, false);
-    private static final ByteList FORMATTED_ANY =
-            new ByteList(new byte[] { 'A','N','Y' }, false);
+    private static final ByteList FORMATTED_DEBUG = ByteList.create("DEBUG");
+    private static final ByteList FORMATTED_INFO = ByteList.create("INFO");
+    private static final ByteList FORMATTED_WARN = ByteList.create("WARN");
+    private static final ByteList FORMATTED_ERROR = ByteList.create("ERROR");
+    private static final ByteList FORMATTED_FATAL = ByteList.create("FATAL");
+    private static final ByteList FORMATTED_ANY = ByteList.create("ANY");
 
     private static ByteList formatSeverity(final int severity) {
         switch ( severity) {

src/main/java/org/jruby/rack/ext/Response.java

@@ -362,7 +362,7 @@ public IRubyObject write_headers(final ThreadContext context, final IRubyObject
         return context.nil;
     }
 
-    private static final ByteList NEW_LINE = new ByteList(new byte[] { '\n' }, false);
+    private static final ByteList NEW_LINE = ByteList.create("\n");
 
     protected void writeHeaders(final RackResponseEnvironment response) {
         this.headers.visitAll(currentContext(), new RubyHash.Visitor() { // headers.each { |key, val| }
@@ -386,7 +386,7 @@ public void visit(final IRubyObject key, final IRubyObject val) {
                     } // else will do addHeader
                 }
 
-                if ( name.equals("Transfer-Encoding") ) {
+                if ( name.equalsIgnoreCase("Transfer-Encoding") ) {
                     if ( skipEncodingHeader(val) ) return;
                 }
 
@@ -566,9 +566,8 @@ public IRubyObject chunked_p(final ThreadContext context) {
         return context.runtime.newBoolean( isChunked() );
     }
 
-    private static final ByteList TRANSFER_ENCODING = new ByteList(
-        new byte[] { 'T','r','a','n','s','f','e','r','-','E','n','c','o','d','i','n','g' },
-    false);
+    private static final ByteList TRANSFER_ENCODING = ByteList.create("Transfer-Encoding");
+    private static final ByteList TRANSFER_ENCODING_LOWER = ByteList.create("transfer-encoding");
 
     private Boolean chunked;
 
@@ -578,15 +577,26 @@ public IRubyObject chunked_p(final ThreadContext context) {
     public boolean isChunked() {
         if ( chunked != null ) return chunked;
         if ( this.headers != null ) {
-            final RubyString key = RubyString.newString(getRuntime(), TRANSFER_ENCODING);
-            final IRubyObject value = this.headers.callMethod("[]", key);
+            final IRubyObject value = getHeaderValue(TRANSFER_ENCODING, TRANSFER_ENCODING_LOWER);
             if ( value instanceof RubyString ) {
                 return chunked = ( (RubyString) value ).getByteList().equal(CHUNKED);
             }
         }
         return chunked = Boolean.FALSE;
     }
 
+    /**
+     * Rack does not mandate response header name casing - apps might use the
+     * conventional Capitalized-Names or (Rack 3.x style) lower-case names.
+     */
+    private IRubyObject getHeaderValue(final ByteList canonicalName, final ByteList lowerCaseName) {
+        IRubyObject value = this.headers.callMethod("[]", RubyString.newString(getRuntime(), canonicalName));
+        if ( value.isNil() ) {
+            value = this.headers.callMethod("[]", RubyString.newString(getRuntime(), lowerCaseName));
+        }
+        return value;
+    }
+
     /**
      * @return whether de-chunking (a chunked Rack response) should be performed
      */
@@ -599,19 +609,17 @@ public IRubyObject flush_p(final ThreadContext context) {
         return context.runtime.newBoolean( doFlush() );
     }
 
-    private static final ByteList CONTENT_LENGTH = new ByteList(
-        new byte[] { 'C','o','n','t','e','n','t','-','L','e','n','g','t','h' },
-    false);
+    private static final ByteList CONTENT_LENGTH = ByteList.create("Content-Length");
+    private static final ByteList CONTENT_LENGTH_LOWER = ByteList.create("content-length");
 
     /**
      * @return whether output (body) should be flushed after each written line
      */
     protected boolean doFlush() {
         if ( isChunked() ) return true;
         if ( this.headers != null ) {
-            final RubyString key = RubyString.newString(getRuntime(), CONTENT_LENGTH);
-            final IRubyObject value = this.headers.callMethod("[]", key);
-            return value.isNil(); // does not have a Content-Length header
+            // does not have a Content-Length header :
+            return getHeaderValue(CONTENT_LENGTH, CONTENT_LENGTH_LOWER).isNil();
         }
         return false;
     }
@@ -645,7 +653,7 @@ protected boolean isClientAbortException(final Exception ioe) {
         return false;
     }
 
-    private static final ByteList CHUNKED = new ByteList(new byte[] { 'c','h','u','n','k','e','d' }, false);
+    private static final ByteList CHUNKED = ByteList.create("chunked");
 
     private boolean skipEncodingHeader(final IRubyObject value) {
         if ( dechunk == Boolean.FALSE ) return false;

src/main/ruby/jruby/rack/error_app.rb

@@ -105,7 +105,7 @@ def map_error_code(exc)
         end
       end
 
-      def respond(status = nil, body = nil, headers = DEFAULT_HEADERS)
+      def respond(status = nil, body = nil, headers = DEFAULT_HEADERS.dup)
         status ||= DEFAULT_RESPONSE_CODE
         body += "\n" if body
         headers['Content-Type'] = "text/plain" unless headers.key?('Content-Type')

src/main/ruby/jruby/rack/error_app/show_status.rb

@@ -21,6 +21,8 @@ def call(env)
       detail = env['rack.showstatus.detail']
       # client or server error, or explicit message
       if (status.to_i >= 400 && empty) || detail
+        # SPEC: when replacing the body the original one needs to be closed :
+        body.close if body.respond_to?(:close)
         # required erb template variables (captured with binding) :
         request = req = ::Rack::Request.new(env); request && req # avoid un-used warning
         message = ::Rack::Utils::HTTP_STATUS_CODES[status.to_i] || status.to_s

src/spec/ruby/jruby/rack/error_app_spec.rb

@@ -82,6 +82,16 @@
     end
   end
 
+  it "returns a fresh headers hash for each response" do
+    init_exception
+    response1 = error_app.call(@env)
+    response1[1]['X-Polluted'] = 'leaked'
+
+    response2 = error_app.call(@env)
+    expect(response2[1]).to_not include 'X-Polluted'
+    expect(JRuby::Rack::ErrorApp::DEFAULT_HEADERS).to be_empty
+  end
+
   it spec = "still serves when retrieving exception's message fails" do
     @env['HTTP_ACCEPT'] = '*/*'
     @env[JRuby::Rack::ErrorApp::EXCEPTION] = InitException.new spec
@@ -133,6 +143,18 @@ def message
       expect(@env['rack.showstatus.detail']).to be false
     end
 
+    it "closes the original body when replacing it with the rendered template" do
+      body = double('body', :each => nil)
+      expect(body).to receive(:close)
+
+      app = lambda { |env| [ 500, {}, body ] }
+      show_status = JRuby::Rack::ErrorApp::ShowStatus.new(app)
+      @env['HTTP_ACCEPT'] = '*/*'
+
+      response = show_status.call(@env)
+      expect(response[2][0]).to include 'Internal Server Error'
+    end
+
     it "with response < 400 and 'rack.showstatus.detail' set to false does not render exception" do
       @env['HTTP_ACCEPT'] = '*/*'; init_exception
       @env['rack.showstatus.detail'] = false

src/spec/ruby/jruby/rack/response_spec.rb

@@ -137,6 +137,15 @@ class << value
     expect(response.chunked?).to be true
   end
 
+  it "detects a chunked response with a lower-case transfer-encoding header" do
+    headers = { "transfer-encoding" => "chunked" }
+    response = JRuby::Rack::Response.new [200, headers, ['body']]
+    # NOTE: servlet container auto handle chunking when flushed no need to set :
+    expect(servlet_response).not_to receive(:addHeader).with("transfer-encoding", "chunked")
+    response.write_headers(response_environment)
+    expect(response.chunked?).to be true
+  end
+
   describe "#write_body" do
 
     let(:stream) do
@@ -304,6 +313,16 @@ class << value
       response.write_body(response_environment)
     end
 
+    it "does not flush the body when lower-case content-length set" do
+      headers = { "content-length" => 10 }
+      response = JRuby::Rack::Response.new [200, headers, ['hello', 'there']]
+
+      response.write_headers(response_environment)
+
+      expect(stream).to receive(:flush).never
+      response.write_body(response_environment)
+    end
+
     it "writes the body to the servlet response" do
       response = JRuby::Rack::Response.new [200, {}, ['1', '2', '3']]
 

src/spec/ruby/rack/input_spec.rb

@@ -39,6 +39,17 @@ def it_should_behave_like_rack_input
         expect(input.read(16)).to eq "hello\r\ngoodbye"
       end
 
+      it "should return an empty string for read(0) without consuming input" do
+        expect(input.read(0)).to eq ""
+        expect(input.read).to eq "hello\r\ngoodbye"
+      end
+
+      it "should replace buffer contents with an empty string for read(0, buffer)" do
+        buf = "cruft"
+        expect(input.read(0, buf)).to eq ""
+        expect(buf).to eq ""
+      end
+
       it "should read into a provided buffer" do
         buf = ""
         input.read(nil, buf)