Skip to content
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

Detect ivars in Rack middleware #15

Open
synth opened this issue Sep 20, 2022 · 4 comments · May be fixed by #52
Open

Detect ivars in Rack middleware #15

synth opened this issue Sep 20, 2022 · 4 comments · May be fixed by #52

Comments

@synth
Copy link

synth commented Sep 20, 2022

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
Copy link
Collaborator

viralpraxis commented Sep 9, 2024

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
Copy link
Author

synth commented Sep 9, 2024

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
Copy link
Collaborator

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

@synth
Copy link
Author

synth commented Sep 11, 2024

ok thanks!

@viralpraxis viralpraxis linked a pull request Sep 22, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants