diff --git a/CHANGELOG.md b/CHANGELOG.md index 920f2007e..8a9b46b42 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/main/java/org/jruby/rack/ext/Input.java b/src/main/java/org/jruby/rack/ext/Input.java index da3431925..c24dd3919 100644 --- a/src/main/java/org/jruby/rack/ext/Input.java +++ b/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); diff --git a/src/main/java/org/jruby/rack/ext/Logger.java b/src/main/java/org/jruby/rack/ext/Logger.java index 36d5659e4..94cfc0fe2 100644 --- a/src/main/java/org/jruby/rack/ext/Logger.java +++ b/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) { diff --git a/src/main/java/org/jruby/rack/ext/Response.java b/src/main/java/org/jruby/rack/ext/Response.java index 7b90692df..8c9bee154 100644 --- a/src/main/java/org/jruby/rack/ext/Response.java +++ b/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,8 +577,7 @@ 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); } @@ -587,6 +585,18 @@ public boolean isChunked() { 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,9 +609,8 @@ 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 @@ -609,9 +618,8 @@ public IRubyObject flush_p(final ThreadContext context) { 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; diff --git a/src/main/ruby/jruby/rack/error_app.rb b/src/main/ruby/jruby/rack/error_app.rb index ed29a8467..ec19dd4ed 100644 --- a/src/main/ruby/jruby/rack/error_app.rb +++ b/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') diff --git a/src/main/ruby/jruby/rack/error_app/show_status.rb b/src/main/ruby/jruby/rack/error_app/show_status.rb index 605d61a24..89e012bae 100644 --- a/src/main/ruby/jruby/rack/error_app/show_status.rb +++ b/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 diff --git a/src/spec/ruby/jruby/rack/error_app_spec.rb b/src/spec/ruby/jruby/rack/error_app_spec.rb index 65302d959..2085be31c 100644 --- a/src/spec/ruby/jruby/rack/error_app_spec.rb +++ b/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 diff --git a/src/spec/ruby/jruby/rack/response_spec.rb b/src/spec/ruby/jruby/rack/response_spec.rb index ca61651e5..eb44b8731 100644 --- a/src/spec/ruby/jruby/rack/response_spec.rb +++ b/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']] diff --git a/src/spec/ruby/rack/input_spec.rb b/src/spec/ruby/rack/input_spec.rb index c132cbab1..aae4b5139 100644 --- a/src/spec/ruby/rack/input_spec.rb +++ b/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)