Introduce Request::ALLOWED_SCHEMES and deprecate existing constant

[JuanitoFatas]

Problem

Update the constant to use clearer terminology.

Description:

Deprecates the constant, provide a new class method Request.scheme_allowlist for anyone who still needs it.

Original motivation, other community efforts examples:

• [ci skip] change all instances of blacklist and whitelist to denylist… rails/rails#33681
• deserialize_jsonapi not found on controller graphiti-api/graphiti#10
• Finish converting whitelist and blacklist references rails/rails#33718
• Use friendlier terminology in YAML.safe_load ruby/psych#378
• Use friendlier terminology in rubysocket.h ruby/ruby#2008
• Use friendlier terminology in _leaf_helpers.erb ruby/ruby#2009
• Use permitted in line_length.rb rubocop/rubocop#6464
• Update friendlier terminology in Gem::SafeYAML rubygems/rubygems#2463
• Update terminologies dtao/safe_yaml#93
• Update terminology in http_status.rb rubocop/rubocop#6466
• Update terminology in rails/validation.rb rubocop/rubocop#6467
• Update terminology in BUILD_DETAIL.md rspec/rspec-core#2576
• Update terminology in BUILD_DETAIL.md rspec/rspec-expectations#1083
• Update terminology in BUILD_DETAIL.md rspec/rspec-mocks#1246
• Update terminology in BUILD_DETAIL.md rspec/rspec-support#356
• Use safelist(s), allowlist(s) where applicable flavorjones/loofah#158
• Use clearer terminologies pry/pry#1874
• Update terminology in test description from completion spec pry/pry#1875

[tonytonyjan]

The term whitelist/blacklist has nothing to do with racist.

[mikegee]

In my opinion, "allowed_schemes" is a bit clearer than "scheme_allowlist".

[rafaelfranca]

I think we don't need a class method for this. This constant was never made to be exposed as public. Just a public ALLOWED_SCHEMAS is fine.

[mikegee]

I think these are called "schemes", not "schemas".

https://www.iana.org/assignments/uri-schemes/uri-schemes.xhtml

[rafaelfranca]

Good point @mikegee. @JuanitoFatas can you rename it?

[JuanitoFatas]

Updated and trailing spaces in Changelog got trimmed by my editor.

[rafaelfranca]

I just realized that this will always warn even if SCHEME_WHITELIST is not used by the app. Is that desired?

[JuanitoFatas]

I did some experiments:

class Namespace
  ALLOWED_SCHEMES = %w(http https)

  SCHEME_WHITELIST = ALLOWED_SCHEMES
  if $VERBOSE
    if Object.respond_to?(:deprecate_constant)
      deprecate_constant :SCHEME_WHITELIST
    else
      warn("SCHEME_WHITELIST is deprecated. Please use ALLOWED_SCHEMES instead.")
    end
  end
end

puts "Reference ALLOWED_SCHEMES"
Namespace::ALLOWED_SCHEMES

puts "Reference SCHEME_WHITELIST"
Namespace::SCHEME_WHITELIST

=>

$ ruby -w namespace.rb
Reference ALLOWED_SCHEMES
Reference SCHEME_WHITELIST
1.rb:19: warning: constant Namespace::SCHEME_WHITELIST is deprecated

Only when we actually reference SCHEME_WHITELIST and $VERBOSE is on will emit warnings, loading the file will not emit warnings.

[rafaelfranca]

But is not that because deprecate_constant is defined? If you don't have it it happens as soon that like 27 is executed

[JuanitoFatas]

Ah I understand now. Yes, for Ruby < 2.3.0, if they turn on warning (-w, --verbose, RUBYOPT=-w), then a warning will show as soon as L27 executed. What do you suggest?

[rafaelfranca]

I'd say, let's remove the else branch for now. There is a chance that rack master will not support Ruby 2.2

[matthewd]

I really like the new name (thanks @mikegee)

[matthewd]

Do we need to check $VERBOSE here? That sounds like it should be deprecate_constant's problem.

[matthewd]

I think this would be clearer phrased as "has been renamed to"; the deprecation behaviour seems the less interesting part. (I'll also note we aren't helping anyone who's changing/overriding the constant, which feels at least as likely as someone using the constant to get ["http", "https"]. But that's probably fine.)

[JuanitoFatas]

Thanks everyone for comment and reviews 🙇