Enhance Request #123

aaoafk · 2025-01-16T20:54:56Z

hello,

i think this should mostly work unless i've missed something.

the only issue right now is maybe remote_ip needs to be thought through a little more and the current method of checking the http action does not verify against HTTP_METHODS I just need to port that underscore method over or just do something else.

lmk your initial thoughts

rsamoilov

I've left some comments, but this looks great!

lib/rage/request.rb

rsamoilov · 2025-01-19T21:04:39Z

lib/rage/request.rb

+  def remote_ip(*args)
+    @env['HTTP_X_FORWARDED_FOR']
+  end


I agree this is going to be more complicated. I would encourage you to work on remote_ip in a separate PR, but that's up to you.

Essentially, we want the algorithm to be close to what it is in Rails with the ability to customize the list of trusted proxies.

I realized that rack already includes behavior for this:

def_delegator :@rack_request, :ip, :remote_ip

It's in ip method. Here I aliased it as remote_ip but that seems to be what rails is relying on for that behavior.

In terms of customizing the list of trusted proxies, I think we could just hook into rack behavior:

trusted_proxies = Regexp.union( /\A127#{valid_ipv4_octet}{3}\z/, # localhost IPv4 range 127.x.x.x, per RFC-3330 /\A::1\z/, # localhost IPv6 ::1 /\Af[cd][0-9a-f]{2}(?::[0-9a-f]{0,4}){0,7}\z/i, # private IPv6 range fc00 .. fdff /\A10#{valid_ipv4_octet}{3}\z/, # private IPv4 range 10.x.x.x /\A172\.(1[6-9]|2[0-9]|3[01])#{valid_ipv4_octet}{2}\z/, # private IPv4 range 172.16.0.0 .. 172.31.255.255 /\A192\.168#{valid_ipv4_octet}{2}\z/, # private IPv4 range 192.168.x.x /\Alocalhost\z|\Aunix(\z|:)/i, # localhost hostname, and unix domain sockets ) self.ip_filter = lambda { |ip| trusted_proxies.match?(ip) }

I'm thinking either add another Regexp.union with rack and a list that we pass down or modify self.ip_filter?

Both options work. I guess modifying self.ip_filter would be somewhat more expected.

I modified the initialize method for request is that what you had in mind for custom proxy support?

Pretty much, but it should be part of the configuration instead, same way it is in Rails - https://guides.rubyonrails.org/configuring.html#actiondispatch-remoteip.

I'll look into what I imagine to be that little configuration object but if memory serves correct I'm not sure that this module is being used anymore ill take another look, because that module resolves the remote ip but ActionDispatch request is forwarding it to rack

aaoafk · 2025-01-21T15:37:14Z

Also one thing I added to cover format as defined in rails api is just:

  def_delegator :@rack_request, :content_type, :format

aaoafk · 2025-01-21T15:38:48Z

As an aside. I noticed that we have url implemented inside: Rage::Request like so:

  # Returns the full URL of the request.
  # @example
  #   request.url # => "https://example.com/users?show_archived=true"
  def url
    scheme = @env["rack.url_scheme"]
    host = @env["SERVER_NAME"]
    port = @env["SERVER_PORT"]
    path = @env["PATH_INFO"]
    query_string = @env["QUERY_STRING"]

    port_part = (scheme == "http" && port == "80") || (scheme == "https" && port == "443") ? "" : ":#{port}"
    query_part = query_string.empty? ? "" : "?#{query_string}"

    "#{scheme}://#{host}#{port_part}#{path}#{query_part}"
  end

Is there a reason we don't want to use url behavior defined inside rack?

      # Tries to return a remake of the original request URL as a string.
      def url
        base_url + fullpath
      end

rsamoilov · 2025-01-22T20:18:41Z

As an aside. I noticed that we have url implemented inside: Rage::Request like so:

  # Returns the full URL of the request.
  # @example
  #   request.url # => "https://example.com/users?show_archived=true"
  def url
    scheme = @env["rack.url_scheme"]
    host = @env["SERVER_NAME"]
    port = @env["SERVER_PORT"]
    path = @env["PATH_INFO"]
    query_string = @env["QUERY_STRING"]

    port_part = (scheme == "http" && port == "80") || (scheme == "https" && port == "443") ? "" : ":#{port}"
    query_part = query_string.empty? ? "" : "?#{query_string}"

    "#{scheme}://#{host}#{port_part}#{path}#{query_part}"
  end

Is there a reason we don't want to use url behavior defined inside rack?

      # Tries to return a remake of the original request URL as a string.
      def url
        base_url + fullpath
      end

No real reason. Feel free to update it.

lib/rage/request.rb

rsamoilov · 2025-01-26T17:29:07Z

Hey @aaoafk , really great job so far! Sorry I wasn't able to provide prompt feedback, but I promise I will get better 😄

aaoafk · 2025-01-26T17:47:13Z

Hey @aaoafk , really great job so far! Sorry I wasn't able to provide prompt feedback, but I promise I will get better 😄

that's fine im more so just trying to get better at programming so your reviews are helping no pressure im working on this in my free time lol

rsamoilov · 2025-02-10T20:26:11Z

Hey @aaoafk , how's it going? Is there anything I can help you with?

aaoafk · 2025-02-11T01:23:04Z

Hey @rsamoilov recently had a big life change, moved to Puerto Rico for work so I was pretty busy the last couple weeks sorting everything out. I'll try to take a look at this soon.

aaoafk · 2025-02-12T23:31:30Z

I committed changes for you to view I still need to look at rack.methodoverride.original_method and the custom_proxies stuff

lib/rage/request.rb

rsamoilov · 2025-02-13T18:42:16Z

I committed changes for you to view I still need to look at rack.methodoverride.original_method and the custom_proxies stuff

It lacks YARD docs, but apart from that, this bit looks good 👍

Co-authored-by: Roman Samoilov <[email protected]>

aaoafk · 2025-02-14T22:16:42Z

I'll look at adding YARD Docs and the custom proxies bit then

aaoafk · 2025-02-17T17:23:55Z

@rsamoilov are you imagining that the custom_proxies bit is a part of config.server.custom_proxies or config.custom_proxies I think the former seems properly scoped

rsamoilov · 2025-02-18T20:39:55Z

@rsamoilov are you imagining that the custom_proxies bit is a part of config.server.custom_proxies or config.custom_proxies I think the former seems properly scoped

config.server is more about the server process - we can use it to set the port the server is running on or the number of workers. How about we add a new config.http scope and put custom_proxies (should actually be trusted_proxies) in there?

rsamoilov · 2025-03-11T20:04:06Z

Hey @aaoafk , are you still working on the PR?

If you are struggling with the remote_ip stuff, I would encourage you to move it to a different PR because everything else is pretty much ready to merge.

aaoafk · 2025-03-11T20:06:06Z

Yeah I was thinking it might take a little bit for that one I'll remove the remote ip stuff and do a push tonight

just focused ones. Commented out the focus run option for rspec

aaoafk · 2025-03-13T00:09:52Z

Hey Roman. I think this should have everything around the remote_ip removed and I corrected the merge conflict, I think we just wanted to take changes from both files since they weren't really conflicting.

rsamoilov

Hey Steven, I've left several comments to fix the tests.

Could you also fix the style warnings? Once it's done, the PR will be good to go.

lib/rage/request.rb

rsamoilov · 2025-03-13T18:56:42Z

spec/rage/request_spec.rb

+  it "handles the protocol property of a request" do
+    expect(request.protocol).not_to be_nil
+  end


Did you remove the protocol method from the Request class on purpose? This test is currently failing because there's no such method.

Strange. I added it back. I don't remember deleting it.

aaoafk · 2025-03-15T03:33:35Z

One test is failing now and I think it might be due to a stale value for the SERVER_PORT inside this test:

  it "handles default ports in URL" do
    env["SERVER_PORT"] = "80"
    expect(request.url).to eq("http://localhost/users?show_archived=true")

    env["rack.url_scheme"] = "https"
    env["SERVER_PORT"] = "443"
    expect(request.url).to eq("https://localhost/users?show_archived=true")
  end

The output is:

expected: "http://localhost/users?show_archived=true"
got: "http://localhost:3000/users?show_archived=true"

I think this is because of the memoized request, it is using an old value for env, since the test does a direct modification on env it wouldn't be reflected there, but some rack methods do query headers directly from env so...

As an aside ill probably re-write the tests to check for literal values instead of just checking not nil...

rsamoilov · 2025-03-16T14:32:40Z

Aha, good catch! Those tests should use contexts instead. Do you want me to fix those?

aaoafk · 2025-03-16T14:48:06Z

Do you mean rspec contexts ?

rsamoilov · 2025-03-16T16:14:55Z

Yep.

ruby 3.1

aaoafk · 2025-03-16T20:40:07Z

How does this look? BTW the previous issue was because of 'HTTP_HOST' header being set to 'localhost:3000'. It needs to be set each time the test is using that for context otherwise it pulls from SERVER_* headers

rsamoilov

🚢 Thank you!

rsamoilov · 2025-03-17T17:00:15Z

Rubocop is failing, but it looks like it's picked up the wrong commit.

aaoafk · 2025-03-17T18:31:10Z

🔥 I'll see about remote ip next or something else curious about the implementation

enhance request

78d0bf8

rsamoilov reviewed Jan 19, 2025

View reviewed changes

Added changes from PR

687006d

rsamoilov reviewed Jan 26, 2025

View reviewed changes

lib/rage/request.rb Outdated Show resolved Hide resolved

lib/rage/request.rb Outdated Show resolved Hide resolved

Go with rack_request explicit methods

e993b4c

rsamoilov reviewed Feb 13, 2025

View reviewed changes

aaoafk and others added 4 commits February 13, 2025 18:15

Update lib/rage/request.rb

b43fc53

Co-authored-by: Roman Samoilov <[email protected]>

Update lib/rage/request.rb

9e888bb

Co-authored-by: Roman Samoilov <[email protected]>

Update lib/rage/request.rb

3adb8f1

Co-authored-by: Roman Samoilov <[email protected]>

Update lib/rage/request.rb

f69b0e0

Co-authored-by: Roman Samoilov <[email protected]>

Add yard changes

6194241

Final push. Remove remote_ip, update request_spec to run all tests not

ca05f20

just focused ones. Commented out the focus run option for rspec

rsamoilov reviewed Mar 13, 2025

View reviewed changes

Update spec, add protocol back to request, rubocop, require set for

cda45fd

ruby 3.1

aaoafk force-pushed the enhance-request branch from b651e78 to cda45fd Compare March 16, 2025 20:38

rsamoilov approved these changes Mar 17, 2025

View reviewed changes

rsamoilov merged commit 64670b3 into rage-rb:master Mar 17, 2025
14 of 16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance Request #123

Enhance Request #123

aaoafk commented Jan 16, 2025

rsamoilov left a comment

rsamoilov Jan 19, 2025

aaoafk Jan 21, 2025

rsamoilov Jan 22, 2025

aaoafk Jan 23, 2025 •

edited

Loading

rsamoilov Jan 26, 2025

aaoafk Jan 26, 2025

aaoafk commented Jan 21, 2025

aaoafk commented Jan 21, 2025

rsamoilov commented Jan 22, 2025

rsamoilov commented Jan 26, 2025

aaoafk commented Jan 26, 2025

rsamoilov commented Feb 10, 2025

aaoafk commented Feb 11, 2025

aaoafk commented Feb 12, 2025

rsamoilov commented Feb 13, 2025

aaoafk commented Feb 14, 2025

aaoafk commented Feb 17, 2025

rsamoilov commented Feb 18, 2025

rsamoilov commented Mar 11, 2025

aaoafk commented Mar 11, 2025

aaoafk commented Mar 13, 2025

rsamoilov left a comment

rsamoilov Mar 13, 2025

aaoafk Mar 15, 2025

aaoafk commented Mar 15, 2025 •

edited

Loading

rsamoilov commented Mar 16, 2025

aaoafk commented Mar 16, 2025

rsamoilov commented Mar 16, 2025

aaoafk commented Mar 16, 2025 •

edited

Loading

rsamoilov left a comment

rsamoilov commented Mar 17, 2025

aaoafk commented Mar 17, 2025

Enhance Request #123

Enhance Request #123

Conversation

aaoafk commented Jan 16, 2025

rsamoilov left a comment

Choose a reason for hiding this comment

rsamoilov Jan 19, 2025

Choose a reason for hiding this comment

aaoafk Jan 21, 2025

Choose a reason for hiding this comment

rsamoilov Jan 22, 2025

Choose a reason for hiding this comment

aaoafk Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

rsamoilov Jan 26, 2025

Choose a reason for hiding this comment

aaoafk Jan 26, 2025

Choose a reason for hiding this comment

aaoafk commented Jan 21, 2025

aaoafk commented Jan 21, 2025

rsamoilov commented Jan 22, 2025

rsamoilov commented Jan 26, 2025

aaoafk commented Jan 26, 2025

rsamoilov commented Feb 10, 2025

aaoafk commented Feb 11, 2025

aaoafk commented Feb 12, 2025

rsamoilov commented Feb 13, 2025

aaoafk commented Feb 14, 2025

aaoafk commented Feb 17, 2025

rsamoilov commented Feb 18, 2025

rsamoilov commented Mar 11, 2025

aaoafk commented Mar 11, 2025

aaoafk commented Mar 13, 2025

rsamoilov left a comment

Choose a reason for hiding this comment

rsamoilov Mar 13, 2025

Choose a reason for hiding this comment

aaoafk Mar 15, 2025

Choose a reason for hiding this comment

aaoafk commented Mar 15, 2025 • edited Loading

rsamoilov commented Mar 16, 2025

aaoafk commented Mar 16, 2025

rsamoilov commented Mar 16, 2025

aaoafk commented Mar 16, 2025 • edited Loading

rsamoilov left a comment

Choose a reason for hiding this comment

rsamoilov commented Mar 17, 2025

aaoafk commented Mar 17, 2025

aaoafk Jan 23, 2025 •

edited

Loading

aaoafk commented Mar 15, 2025 •

edited

Loading

aaoafk commented Mar 16, 2025 •

edited

Loading