search

ohler55 / agoo

A High Performance HTTP Server for Ruby
MIT License
912 stars 39 forks source link

REMOTE_ADDR not set on ENV #98

Closed texpert closed 4 years ago

texpert commented 4 years ago

Hello, @ohler55 !

First, let me thank you for your work on this very interesting server! I am trying it running by Rails and I am really impressed how the memory consumption is staying flat!

But then, I have discovered the broken authentication, and tracked the cause to be the missing REMOTE_ADDR. Rack is trying to fetch it from ENV, but it is missing. I see how Puma is setting it https://github.com/puma/puma/blob/master/lib/puma/dsl.rb#L739, but found no such code in Agoo.

texpert commented 4 years ago

UPDATE: I've double-checked, and authentication actually works on production, because Rack is also trying to fetch the request.ip from the HTTP_X_FORWARDED_FOR, which, as I understand, is provided by nginx or whatever server stays in front of Heroku instance with Agoo and Rails.

Still a problem in the development, or any other environment not providing HTTP_X_FORWARDED_FOR.

ohler55 commented 4 years ago

I’ll look into that later today.

ohler55 commented 4 years ago

I could not find any mention of REMOTE_ADDR in the Rack spec at https://github.com/rack/rack/blob/master/SPEC.rdoc. I have no problem adding something though if it is commonly used. Is there are definition somewhere on the rules for setting the value?

texpert commented 4 years ago

Yes, the rules are defined by the CGI RFC-3875 - https://tools.ietf.org/html/rfc3875#section-4.1.8

texpert commented 4 years ago

Also, here's a good SO response on the matter - https://stackoverflow.com/a/43014286/4812102

ohler55 commented 4 years ago

Great, thanks. That helps.

ohler55 commented 4 years ago

Interesting that REMOTE_ADDR does not include the HTTP_ prefix as would be expected from the spec. I don't see any other special cases like that except for some RACK_ prefixed keys. Are you aware of any others that should be added? It also does not appear as if all the RFC 3875 environment variables are needed for the Rack::Request.

texpert commented 4 years ago

Yeah, this seems to be a special case, I am not aware of any other. As for the HTTP_ prefix, it is not very clear from the spec, but it seems it should not be applied to the explicitly listed in the spec variables. And, well, this spec is not a standard, as it is stated at the document start, but just a current practice of CGI.

ohler55 commented 4 years ago

The 'remote-add' branch has a candidate fix to this issue. Please give it a try.

texpert commented 4 years ago

Great, works like a charm!

ohler55 commented 4 years ago

Great, I'll add a few tests more tests and release.

ohler55 commented 4 years ago

Released

texpert commented 4 years ago

Thank you very much, @ohler55 !! 👍