Issue #22233 has been reported by adam.sligar@getquip.com (Adam Sligar). ---------------------------------------- Feature #22233: Warn when Net::HTTP#set_debug_output enables debug output https://bugs.ruby-lang.org/issues/22233 * Author: adam.sligar@getquip.com (Adam Sligar) * Status: Open ---------------------------------------- # Abstract `Net::HTTP#set_debug_output` logs every request and response in plain text, including `Authorization` and `Cookie` headers and message bodies. Its documentation says never to use it in production, and nothing enforces that. I propose writing a warning to `$stderr` when debug output is enabled, at most once per thread. # Background The method has carried this documentation for as long as it has existed: # *WARNING* This method opens a serious security hole. # Never use this method in production code. The advice is sound and there is nothing behind it. The method behaves identically in a console and in a production worker, so debug output committed by accident, or enabled by hand during an incident and never reverted, keeps writing credentials to whatever stream it was given. The credentials are then retained and indexed. The only warning the method has today concerns call ordering, not safety: warn 'Net::HTTP#set_debug_output called after HTTP started', uplevel: 1 if started? # Proposal Warn when `set_debug_output` is enabled: $ ruby -rnet/http -e 'Net::HTTP.new("example.com").set_debug_output($stdout)' -e:1: warning: Net::HTTP#set_debug_output: every request and response, including Authorization and Cookie headers and message bodies, will be written to the given stream in plain text; never enable this in production Nothing is written when: * `$DEBUG` is set. Ruby already has a flag meaning "this process is being debugged", and enabling wire logging under it is deliberate. `net/http`'s own test harness reads it the same way, in `test/net/http/utils.rb`. * `output` is `nil`, which disables debug output rather than enabling it. * the warning has already been written on this thread. The once-per-thread flag lives in thread-local storage. `set_debug_output` is a configuration setter that `net/http` never calls itself, so a connection configured once and reused pays nothing per request. But wrappers that build a `Net::HTTP` per request and configure it each time would otherwise repeat an identical message on every request and flood the log the warning is meant to draw attention to. Thread-local storage rather than a class-level instance variable because assigning the latter from a non-main Ractor raises `Ractor::IsolationError`, and `Thread#thread_variable_set` has been available since 2.0, so no version guard is needed. # Use cases * A `set_debug_output` call reaches production in a release. Today the first sign is credentials in the log store. * Debug output is enabled while diagnosing a production incident and not reverted. The next deploy carries it forward silently. * A gem enables debug output on a connection it owns. The application never sees the call and today gets no indication that its requests are being logged. # Discussion **Why not detect production and warn only there?** Because Ruby has no concept of an application environment to consult. No file in `lib/` reads `RAILS_ENV`, `RACK_ENV` or `APP_ENV`; there is no `RUBY_ENV`; `RbConfig` describes the build, not the deployment. Reading a framework's variable would put knowledge of that framework into the standard library and force it to pick a side. Rails, which resolves `RAILS_ENV` then `RACK_ENV`, and Sinatra, which resolves `APP_ENV` then `RACK_ENV`, etc. It would also still miss the case most relevant to a standard library: a script with no framework and no variables set. Warning unconditionally and exempting `$DEBUG` avoids all of it and covers more. **Why not gate on `$VERBOSE`?**`$VERBOSE` is false unless `-w` is passed, which would hide this in exactly the unattended runs it exists to catch. The existing "called after HTTP started" warning in this same method is not gated either. **Why not a `Warning` category?** Of the categories Ruby 4.0 defines (`:deprecated`, `:experimental`, `:performance`, `:strict_unused_block`), only `:experimental` is enabled by default, and none of them describes this. A guardrail nobody has enabled does not guard anything. If ruby-core would prefer a category, an always-on `:security` category has precedent in `:experimental` and would be a reasonable separate proposal; I would rather not block this on introducing one. The warning can already be intercepted by overriding `Warning.warn`. **Why not deprecate the method?** It is intended to exist and is useful. `Warning[:deprecated]` is also off by default, so a deprecation would be invisible in production. Feature #19630 used deprecation correctly because that behavior genuinely was scheduled for removal. -- https://bugs.ruby-lang.org/