Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Puma >= 6.1.0 + Rails 7 produce empty rack.input in custom middleware #3145

Closed
sanzstez opened this issue May 10, 2023 · 7 comments
Closed

Comments

@sanzstez
Copy link

sanzstez commented May 10, 2023

I am using custom middleware for parsing multipart/related body content. Request sending with Transfer-Encoding: chunked.
Everything works fine with version of puma <= 6.0.2.
But after upgrade puma to 6.1.0 rails become generate Bad request content body in output. And as I see with debug env['rack.input'] contain empty string.

UPD:
During my research I found that functionality was broken by this PR: #3062
by adding

@io.read_nonblock(4096, @read_buffer)

instead of

@io.read_nonblock(4096)

Puma config:
Default rails DEVELOPMENT config.

To Reproduce
Simplified middleware what I am using:

module BlockPackage
  class Middleware
    def initialize(app)
      @app = app
    end

    def call(env)
      if env['CONTENT_TYPE'] =~ /^multipart\/related/ni
        puts env['rack.input'] # contain empty string "", but contain data after env['rack.input'].rewind
      end

      @app.call(env)
    end
  end
end

Middleware was included to rails app as usually:

Rails.application.config.middleware.use(BlockPackage::Middleware)

Ruby version: 3.1.2
Rails version: 7.0.4.3
Rack version: 2.2.7
Puma version: >= 6.1.0

@sanzstez sanzstez changed the title Puma 6.1.0 + Rails 7 produce empty rack.input in custom middleware Puma >= 6.1.0 + Rails 7 produce empty rack.input in custom middleware May 10, 2023
@willkoehler
Copy link
Contributor

Hi @sanzstez I can help look into this. The first step for me will be to reproduce locally. Is this something you can help with? Maybe we can setup a simple test app based on https://github.com/puma/puma/tree/master/benchmarks/local/sinatra

Any help you can provide helping me reproduce locally would be appreciated. Once I can do that, I should be able to take it from there.

@sanzstez
Copy link
Author

@willkoehler Thank you for response. Really appreciate!
Sorry, I am not sure that can reproduce this behaviour with sinatra app. Maybe can try later at least ...
Rolled back to 6.0.2 version for now.

@willkoehler
Copy link
Contributor

@sanzstez agreed, the Sinatra app might not work because of the Middleware. We could use a simple Rails app perhaps?

@MSP-Greg
Copy link
Member

@sanzstez

PR #3137 added some tests that would seem to cover this issue, but it's only in the master branch. The issue fixed by the PR involved some encoding issues with uploads. They used curl as a client, similar to the below:

"curl -H 'transfer-encoding: chunked' --form data=@#{temp_file_path} http://127.0.0.1:#{@port}/"

Might this be related? If so, can you check this against Puma master?

@willkoehler
Copy link
Contributor

willkoehler commented May 10, 2023

Thanks @MSP-Greg that certainly looks promising!

BTW I originally had @read_buffer = String.new in my PR but felt obligated to convert to @read_buffer = +"" because of the Rubocop warning. It's interesting the unintended side-effects of optimizations like Performance/UnfreezeString I went along with it because it seemed harmless. I would have never anticipated that +"" would introduce a 'character/line-ending' sensitive side effect like that. Nice detective work 🙌

@sanzstez
Copy link
Author

sanzstez commented May 10, 2023

@MSP-Greg @willkoehler I don't know why ... but it works from master! Checked few minutes ago. Can you please explain what happened? (for understand).

UPD: Checked master sources and comments above. Understood. Very interesting story with rubocop and String.new vs +"", really.
Thanks for help!

@dentarg
Copy link
Member

dentarg commented May 11, 2023

Great, #3137 should be part of the next release. I'll close this.

@dentarg dentarg closed this as completed May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants