-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
HTTP::Request, added remote ip and peer_addr #1722
Conversation
|
||
def peer_addr | ||
@peer_addr.not_nil! | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is property! peer_addr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know :)
@@ -68,4 +89,21 @@ class HTTP::Request | |||
private def uri | |||
(@uri ||= URI.parse(@resource)).not_nil! | |||
end | |||
|
|||
private def ips_from(header) | |||
if ips = headers[header]? || headers["Http-#{header.tr("_", "-")}"]? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTTP::Headers
should have independent lookup by now, https://github.com/manastech/crystal/blob/master/spec/std/http/headers_spec.cr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it can't lookup HTTP_X_FORWARDED_FOR and X-Forwarded-For right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So only the tr("_", "-") is redundant right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh, missed the comment, sorry. But yes, you're right and looks good now.
end | ||
|
||
private def trusted_proxy?(ip) | ||
ip =~ /\A127\.0\.0\.1\Z|\A(10|172\.(1[6-9]|2[0-9]|30|31)|192\.168)\.|\A::1\Z|\Afd[0-9a-f]{2}:.+|\Alocalhost\Z|\Aunix\Z|\Aunix:/i |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
127.0.0.0/8
https://tools.ietf.org/html/rfc6890- Should we trust link-local addresses?
fe80::/10
I also wonder given this whether we shouldn't have remote_ip(trust_headers=true, trust_all_clients=false)
? Then docs on that would be great elaborating on what the parameters mean.
I'm not so sure about this. I think we're adding too much intelligence to the Request already. Maybe we need to put this logic in a handler that we insert as a middleware. Also, note that the same class is used for client requests so I wonder if we shouldn't have a context object instead that we pass around between server handlers. |
@waj that was my first suggestion. I only added the peeraddr to the REMOTE_ADDR header to be used in middleware. I like the peer_addr on request better. But perhaps we can move the remote ip logic to a handler that sets the REMOTE_ADDR header? Or is there another way of passing that info down the middleware stack without using the headers? |
I would really dislike adding custom headers. And a middleware approach would do the detection on every single request, keep in mind it's about 10 header checks per request. |
But if you use it for logging purposes you're gonna be doing the lookups on every request either via the middleware or via the request object. But there's always the option of creating an extra class where the logic can be placed. We could also create an HTTP util class. But in all cases do we agree to the peer_addr being set on the request? Cause I would happily split the pull request into peer_addr and remote_ip |
How about having a middleware that calls underlying handlers with request decorator, that has all the additional logic added on top of raw request? i.e.: class SmartHTTPHandler < HTTP::Handler
def call(request)
smart_request = SmartRequest.new(request)
call_next(smart_request)
end
end
class SmartRequest
private getter original_request
def initialize(@original_request)
end
delegate_when_missing_to original_request
# lazily fetches peer address of request
def peer_addr
# ...
end
# set peer address of request
def peer_addr=(value)
# ...
end
# .. and more stuff, like #url_params, #body_params, etc. ..
end EDIT: of course naming is deliberate and requires some thought to make it right. EDIT 2: of course everything, that it adds on top, should be as lazy as possible, to not create a performance overhead for unused parts. EDIT 3: it doesn't necessary need to have all the additional functionality, instead one can have multiple "smart" request decorators and multiple "smart" middlewares allowing one to compose them as needed. |
Other point, is that middleware is not required in this case, one can use these decorators directly on demand. |
We plan to make a review of http request/response and client with @waj for this, and we'll hopefully come with a good solution. |
@sdogruyol It's on our TODO list, just that we won't implement it this way (it doesn't have to be attached to the request, it will probably be in the HTTP::Server::Context). |
I originally came here to make sure this doesn't consider the HTTP headers as the authoritative source for IP address since HTTP headers can be spoofed. Glad to see there is an option to rely on the socket information (coming from the TCP connection data itself) or the HTTP header. As far as code quality is concerned, LGTM. Perhaps some cleanup of the specs according to the comments already left by others. LGTM 👍 /unsolicited-review |
FWIW it would be "convenient" to have this in there I think people would use it. Here's how rails 2.3 does it: https://gist.github.com/rdp/ec8d8d7152988f496f951a2f0d65a320 |
Still not merged!! I really need that right now 😅 @benoist |
Well i think this code is pretty outdated. Happy to make the required changes to get this merged though. |
@benoist Yeah I'd say do it, if not I will in a couple weeks I think... |
Wow, i still remember the day this issue opened. Time really flies 😄 |
I assume just the review comments but everybody feel free to chime in :)
…On Mon, Apr 10, 2017 at 11:36 AM, Benoist ***@***.***> wrote:
@rdp <https://github.com/rdp> Do we know what the required changes are,
as I believe @asterite <https://github.com/asterite> was going to discuss
the implementation details with @waj <https://github.com/waj>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1722 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAw0CrOCBc78bFWJvIHNB-B95GVrWV6ks5rumimgaJpZM4GMm_q>
.
|
That was after the review comments from jhass |
This is too old so I'm closing it. We still need this functionality, though, but it will probably come from the core team. Maybe we can copy what Go does, but I don't know (I don't know much about HTTP and proxies). |
No description provided.