Does storing the ActiveDispatch::Request object make this module bloat memory usage?

[SpamapS]

Hi! I am fairly new to ruby/rails and the like, so I apologize for the somewhat beginner question.

omniauth-rails_csrf_protection/lib/omniauth/rails_csrf_protection/token_verifier.rb

Line 35 in f0f5f84

@request = ActionDispatch::Request.new(env.dup)

This line stores the request object in the token verifier, which is effectively a singleton since it is part of a Singleton.

The trouble I'm seeing is that this prevents GC'ing large request controllers until the next Oauth cycle that hits this particular worker process. With lots of puma workers, and not all that frequent Oauth cycles, this could be a long time. Doing some heap analysis we see this:

Objects retaining the most live memory:
root: 493.2 MB (2417120 objects)
OmniAuth::Configuration[0x7859f37e5cc8][CLASS]: 253.1 MB (1145070 objects)
OmniAuth::Configuration[0x7859f37adb98]: 253.1 MB (1144998 objects)
OmniAuth::RailsCsrfProtection::TokenVerifier[0x7859f482a6d8]: 253.1 MB (1144971 objects)
ActionDispatch::Request[0x7859de0c2118]: 253.1 MB (1144970 objects)
Hash[0x7859de0c2140][size=114]: 253.1 MB (1144967 objects)
Puma::Client[0x7859d99abae0]: 253.0 MB (1144784 objects)
Hash[0x7859d553ef60][size=95]: 253.0 MB (1144778 objects)
<redacted>Controller[0x7859d54b2ec0]: 253.0 MB (1144548 objects)
<redacted>::ActiveRecord_Relation[0x7859d49d3f18]: 243.4 MB (1144279 objects)
...: 2.9 GB (14633727 objects)

Now, this particular API endpoint is the most extreme, as it lacks pagination, and as such has been deprecated. But we have others that use lots of memory briefly for other reasons, and they suffer the same fate.

Is storing the request in the token verifier necessary? Is there a post-request callback that gives us the opportunity to clear it?

I was looking for other examples of validation phase handlers and I only see the original one here which follows a very different pattern:

https://github.com/omniauth/omniauth/blob/e23567ac860f4adfb425db226d68a03235078941/lib/omniauth/authenticity_token_protection.rb

Anyway, really appreciate your attention to this. Thanks!

[sikachu]

Sorry for the late reply.

As mentioned in the README, this gem uses ActionController::RequestForgeryProtection code from Rails. Unfortunately, this means we need an instance of ActionDispatch::Request in the middleware as there're a few methods that are being called on request here.

And yes, I believe that's one of the reason why OmniAuth has a different strategy which is more lightweight comparing to this.

I think back when this gem was introduced, there was some urgency to get it working ASAP as the CVE became public. Now that this gem is widely used, I might spend some time on it to reduce the memory usage.

Anyway, thank you very much for opening this issue. I'll investigate and see what I can do about it.

[SpamapS]

Further analysis has revealed that this is not a likely candidate for preventing the reaping of controllers, but instead a bug that was fixed in Ruby 3.3. This request object is a bespoke request object, where it's just being used as a way to get at the pieces of the request at the rack level, not pulling in information from further along in the application's life. It still probably shouldn't be stored in a singleton if that can be helped, but it's probably a very, very minor memory problem since I don't think there's a way any controllers will get attached to this object.

[nevans]

For what it's worth, although the class acts as a singleton of sorts, #call dups the middleware and @request is only assigned in the copy (since #17). The copy goes out of scope at the end of #call, so it should be garbage collectable at that point.