@rafaelfranca thanks. Are there any plans to release this with patch version bump? I can help to prepare the release. Once released, I can open the last PR to handle this exception properly in Rails.

This to me looks like a breaking change for existing Rack 2.x - @jeremyevans what do you think?

@ioquatix As I have pointed at #1837, specs are passing with no change (even I have changed them to use the new exception). New exception is inherited from the old one. Can you elaborate a little more what do you think is breaking change in here?

Ah yes, I missed that you subclassed RangeError, it might be acceptable then. I think it might have been good to retain the existing specs since that shows that it's still compatible with the original expectations. Looking at the specs, it looks like it's completely different class.

I can add additional spec to ensure ParamsTooDeepError inherits from RangeError if welcomed with small comment explaining on this. That should cover unintentional change of the parent class in the future. WDYT?

I think it's a great idea to prevent someone accidentally refactoring it and not realising it was part of the original specs :)

It might be pedantic to do all the specs, but even just a spec like, ensuring that the class is a subclass of RangeError could be sufficient.

I thought about this a bit more, the only compatibility issue that I can think of would be systems like Honeybadger (or other APMs) which do error reporting, it would mean the class would change and historical data might not align up.. but not sure if I personally care about that much. That being said, it is a user visible change.

Personally I understand and agree with the motiviations of this change, but compatibility needs to be considered carefully, it's not always obvious how it breaks things and people doing minor or patch updates don't necessarily expect it. I've definitely been bitten by stuff like this before in production. Considering how many billions or trillions of requests are handled by Rack daily, the blast radius is pretty big.

@ioquatix currently default Rails app is vulnerable to 500 error (crashing the app on uncaught exception) which could be considered attack vector. Having this custom exception, it would be possible to craft patch to Rails to prevent this and gracefully fail. That's the reason for this backport. Currently it is not possible to release from main branch and update Rails dependency to Rack 3. There is no other option than this (at least I wasn't able to find out any).

A little confused error reporting is not big deal in my eyes, maybe even it is clever enough to group those errors together. No error is going to be hidden, it can just re-appear under different name.

Btw. I have explained all this info to Rafael (indirectly, thru Shopify folks) and that is (probably) the reason to move this forward as soon as possible.

Thanks for that information. I didn't know there was a security component to this. I think the take away here is that a little bit more communication would have been helpful here, the original description doesn't give enough context. For reference, here is the original PR: #1837. Might I suggest in the future you start by creating an issue and then link all the relevant PRs.

The bar for security fixes is certainly lower in terms of compatibility but it still needs to be considered carefully. I don't personally have a stake in this, but my expectation is that @tenderlove probably needs to review this change before we make any release. I would also like to hear input from @jeremyevans since he has a keen sense for compatibility issues.

Like I said earlier, I personally support this PR, but that doesn't mean the bar doesn't need to be met, in fact my approval means very little in this particular space as I think other maintainers are more sensitive to the issues here :)

Replacing an exception class with a subclass of the exception class is generally considered a backwards compatible change, as all rescue clauses will still match. Anything it breaks pretty much deserves to be broken.

I don't consider this a security issue, though I'm sure it is annoying to receive bug reports for 500 errors instead of just reporting 4xx errors to the client. That said, I'm fine with the backport to Rack 2.2.