Gitlab::Utils.to_boolean treats a multi-line string as a boolean when one line is a token

Everyone can contribute. Help move this issue forward while earning points, leveling up and collecting rewards.

Summary

Gitlab::Utils.to_boolean (utils.rb#L92-93) matches its tokens with /^(true|t|yes|y|1|on)$/i and /^(false|f|no|n|0|off)$/i. In Ruby ^ and $ match at every line boundary, so a string with one line equal to a token converts as that token instead of returning the default. The truthy pattern is checked first, so "false\ntrue" returns true.

Its callers pass environment variables, request and API parameters, integration properties and application settings, so a value with an embedded newline is read as true or false depending on which of its lines matches.

Steps to reproduce

  1. Check out master and run bundle install in gems/gitlab-utils.

  2. In that directory, run:

    bundle exec ruby -Ilib -e 'require "gitlab/utils"; ["false\ntrue", "garbage\ntrue", "garbage\noff", "\nyes", "true\n", "fals"].each { |v| puts "#{v.inspect} => #{Gitlab::Utils.to_boolean(v).inspect}" }'

Example Project

Not needed: the method is in the gitlab-utils gem and needs no instance.

What is the current bug behavior?

"false\ntrue", "garbage\ntrue" and "\nyes" return true, and "garbage\noff" returns false. With default: 42, "no\nyes" returns true.

What is the expected correct behavior?

A string that is not one of the tokens returns the default (nil unless one is given), as "fals" already does.

Relevant logs and/or screenshots

On master at ac9e1bae, Ruby 3.3.11:

"false\ntrue" => true
"garbage\ntrue" => true
"garbage\noff" => false
"\nyes" => true
"true\n" => true
"fals" => nil

Output of checks

Reproduced with the gem alone, so there is no instance output to add.

Possible fixes

I propose anchoring both patterns to the whole string with \A and \z, as the Ruby secure coding guidelines recommend. That also makes a value with one trailing newline, such as "true\n", return the default. If some caller relies on that value being accepted, \A and \Z keep it working and change only strings with a newline elsewhere.

Which do you prefer? I have the change and its specs ready for either, and will open the merge request once that is decided.

The draft merge request !260362 copies the same two patterns into gems/gitlab-platform-core/lib/gitlab/instance_identity.rb, so that copy would need the same change.

Edited by 🤖 GitLab Bot 🤖