Fixes #38120 - Support Sinatra 4 and Rack 3 - #955
Conversation
lhellebr
changed the title
|
Converting to draft because we need to investigate how this will work on rpm based deployments with Foreman: it will likely require bump to Rails 7.1. |
|
This is largely a duplicate of #952. Based on #952 (comment) it would be great to separate the Rack 3 compatibility into its own PR to be cherry picked into Foreman 5.0. |
|
Updated so this supports EITHER Sinatra 2 + Rack 2 OR Sinatra 4 + Rack 3. |
| # Needed so `logger` resolves to Proxy::LogBuffer::Decorator (which implements | ||
| # #exception, used in the rescue blocks below) instead of Sinatra's own null | ||
| # logger, which is a plain ::Logger with no #exception method as of Sinatra 4. | ||
| helpers ::Proxy::Helpers |
There was a problem hiding this comment.
Looks like this is a bug we've had that went unnoticed for a long time
|
Note: you'll also need to set See sinatra/sinatra#2053 which defaults to permitting localhost and test domains only. Alternatively forwarding foreman-proxy trusted hosts setting to Sinatra permitted hosts I guess could also work. To be added in all classes that inherit from |
| # enables strict Rack::Protection::HostAuthorization that rejects | ||
| # requests whose Host header isn't localhost — breaking all real | ||
| # deployments where Foreman reaches smart-proxy by hostname. | ||
| ::Sinatra::Base.set :environment, :production |
There was a problem hiding this comment.
This is intended to be a fix for an issue that @ibuclaw raised.
The other option (using trusted_hosts in permitted_hosts context) wouldn't IMO make sense because one is about request origin, the other is about request target.
This line has been there for a long time and I think it was a bug - the correct symbol is :environment https://sinatrarb.com/configuration.html . It seems like it has been unnoticed for a long time. Note that changing environment can have consequences beyond just this fix - different security rules or logging content and others - I'm not certain what exactly. So this change needs to be weighed carefully.
In either case, the test suite is still green and the proxy still seems to work after this change.
There was a problem hiding this comment.
Makes sense, thanks.
That wrong name explains why I couldn't find environment anywhere.
There was a problem hiding this comment.
Thanks for warning me about this, @ibuclaw !
lhellebr
changed the title
michalgritzbach
left a comment
michalgritzbach
left a comment
There was a problem hiding this comment.
Looks good to me, I think that the hardcoded :production env is a sane default. From what I've found, it could potentially hide traces in failed tests - if this would be an issue, adding ::Sinatra::Base.set :raise_errors, true to lib/smart_proxy_for_testing.rb should fix that. But I wouldn't consider it a blocker.
|
@lhellebr could you please fix up the Redmine issue reference in the commit message so that test passes? |
Allow smart-proxy to run on either Sinatra 2 + Rack 2 or Sinatra 4 + Rack 3, depending on what is installed. This enables Debian 13 support (which ships Rack 3) without forcing all platforms to upgrade simultaneously. gemspec: Widen constraints to rack >= 2.0 < 4, sinatra >= 2.0 < 5, add rackup (needed for Rack 3, harmless on Rack 2). launcher.rb: Resolve the WEBrick handler at load time via require/rescue LoadError — Rackup::Handler::WEBrick for Rack 3, Rack::Handler::WEBrick for Rack 2. hsts_middleware.rb: Use unconditional lowercase header key. Rack 3 requires it; Rack 2 does case-insensitive lookups internally so lowercase works for both. registration_api.rb: Add missing helpers ::Proxy::Helpers so logger resolves to Proxy::LogBuffer::Decorator (which implements #exception) instead of Sinatra's built-in logger. Under Sinatra 2 this was masked by Rack::NullLogger swallowing unknown methods; Sinatra 4 uses a real ::Logger that crashes on #exception. smart_proxy_main.rb: Fix Sinatra environment setting from :env (a no-op custom attribute) to :environment (the real Sinatra setting). Without this, Sinatra defaults to development mode, which in Sinatra 4 enables Rack::Protection::HostAuthorization that rejects requests whose Host header isn't localhost — breaking all deployments where Foreman reaches smart-proxy by hostname. Note: Sinatra 4's Rack::Protection::HostAuthorization can restrict accepted Host headers to a configured allow-list, but in production mode it allows all hosts by default. This means we don't yet benefit from the additional security Sinatra 4 offers here. Configuring permitted hosts (e.g. via foremanctl or the installer) should be addressed as follow-up work; for now this is not a regression since Sinatra 2 never had this protection at all.
|
@ekohl done |
ofedoren
left a comment
ofedoren
left a comment
There was a problem hiding this comment.
LGTM as well. Do we consider configuring permitted_hosts in a follow up?
5 participants
Updates bundle to new versions.
In Rack 3, headers must be lowercase rack/rack#1592.
We now use Rackup::... instead of Rack::...
test_global_500 and test_host_500 in RegistrationRegisterApiTest have been failing, both raising
NoMethodError: undefined method 'exception' for #<Logger:...>frommodules/registration/registration_api.rb's error-handling blocks. The cause was thatProxy::Registration::Apinever included helpers::Proxy::Helpers(unlike every sibling API class), so its logger calls resolved through Sinatra's own built-in logger helper rather than smart-proxy's real logger; under Sinatra 2 that helper pointed atRack::NullLoggerwhen logging was disabled, and whileRack::NullLoggernever actually had an #exception method either, the test's Mocha stub on that exact class silently intercepted the call anyway, whereas Sinatra 4 replaced that null-logger path with a plainstdlib ::Logger, which the test's stub no longer targets and which genuinely has no#exceptionmethod, turning a previously-masked bug into a real crash. The fix was addinghelpers ::Proxy::HelperstoProxy::Registration::Api, which makes logger resolve toProxy::LogBuffer::Decorator.instance(which does implement#exception), matching the pattern already used by every other API class in the codebase — after which the full test suite passed.