Detect ivars in Rack middleware

[synth]

Instance variables in Rack middleware are bad. They are not thread safe and are insidious to track down issues relating to it.

Ref:

1. Request responses getting swapped puma/puma#2001
2. https://bernardoamc.com/rails-middleware-leak/
3. https://crypt.codemancers.com/posts/2018-06-07-frozen-middleware-with-rack-freeze/

Since this is such a gotcha, I (via my company) would gladly sponsor development of such a cop to walk the middleware hierarchy and catch offenders which could occur at app level or third party.

[viralpraxis]

Hey @synth, thank you for bringing this up!

As you correctly pointed out, a hypothetical cop would need to "walk the middleware hierarchy," which isn't something static analysis tools like RuboCop are designed to handle. From my understanding, it's impossible to detect Rack middlewares in an application solely through source code analysis. This is because, according to the Rack specification, middleware (which is essentially a Rack application) is just an object with a 1-arity call method that returns a specific array structure. Even if we tried to detect such objects, it would likely lead to numerous false positives and negatives. Additionally, it wouldn’t protect against problematic middlewares introduced by external gems used by the application.

I see three possible solutions:

1. A runtime verification tool that is invoked after the Rack application starts, traversing the middleware stack to check if all middlewares are frozen. This can be implemented as a rails initializer, for instance.
2. rack-freeze (though I'm not sure if it works with Rails or Rack3).
3. We could attempt to implement such a cop by adding a mandatory configuration option that specifies the list of custom middleware locations. However, I'm uncertain if this would significantly improve thread safety.

What do you think?

[synth]

Thanks for getting back. Indeed, none of the solutions seem ideal. I'd personally lean towards option 3, but I'd vote for a directory or glob to be specified as the option so if apps keep their middleware in a common directory, they wouldn't need to know to update the cop configuration to add the new middleware. However, this would breakdown, say, in an app that was composed of a lot of Rails engines that might have their own middleware. I'm not sure if engines have a common path to middleware. We have ours in app/lib/middleware, but no idea if that's standard. Maybe this is better left to some kind of dynamic analyzer and implemented via option #1 as you mentioned (although I don't think I'd want to bloat the initializer stack for it though)

[viralpraxis]

Yep, I think it might make sense to try to implement (partial) solution 3. I'll report here on my progress.

[synth]

ok thanks!